Message ID | ZfO6zKtvp2jSO4vF@gondor.apana.org.au |
---|---|
State | New |
Headers | show |
Series | [GIT,PULL] Crypto Update for 6.9 | expand |
On Thu, 14 Mar 2024 at 20:04, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Drivers: > > - Add queue stop/query debugfs support in hisilicon/qm. There's a lot more than that in there. Fairl ybig Intel qat updates from what I can see, for example. Linus
The pull request you sent on Fri, 15 Mar 2024 11:04:44 +0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.9-p1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c8e769961668ef56acabc67f040c58ed769c57e4
Thank you!
On Fri, Mar 15, 2024 at 02:51:47PM -0700, Linus Torvalds wrote: > On Thu, 14 Mar 2024 at 20:04, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > Drivers: > > > > - Add queue stop/query debugfs support in hisilicon/qm. > > There's a lot more than that in there. Fairl ybig Intel qat updates > from what I can see, for example. Sorry, one line got chopped off while I was creating the signed tag: - Improve error recovery in qat. Cheers,
On Sun, 12 May 2024 at 20:50, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Lukas Wunner (1): > X.509: Introduce scope-based x509_certificate allocation I absolutely hate how this commit tries to remove one single compare instruction by introducing a *very* dangerous hack. The whole 'assume()' thing will generate actively wrong code if that assumption conditional doesn't hold, to the point of being completely impossible to debug. Having random kernel code add random "assume()" lines is absolutely not what we should do. Particularly not in some random code sequence where it absolutely does not matter ONE WHIT. Now, I've pulled this, but I killed that "assume()" hackery in my merge. Because there is no way we will ever encourage random code to make these kinds of patterns, and I most definitely do not want anybody else to try to copy that horrendous thing. Yes, yes, we have "unreachable()" in other places, and yes, you can make compilers generate garbage by using that incorrectly. But they should be about obvious code warning issues, not about "let's save one conditional instruction". Now, if somebody really *really* cares about that one extraneous conditional, particularly if it shows up in some more important place than some random certificate parsing routine where is most definitely is not in the least critical, there are better models for this optimization. Maybe somebody can teach the kernel build in *general* that "kmalloc()" and friends never return an error pointer, only NULL or success? That would not necessarily be a bad idea if the scope-based cleanup otherwise causes issues. But this kind of hacky "one random piece of kernel code uses a very dangerous pattern to state that some *other* piece of kernel code has particular return patterns" - that is not at all acceptable. Put another way: it would probably be ok if the SLAB people added some "this function cannot return error codes" annotation on their core declaration and it fixed an issue in _general_. But it is *not* ok if random kernel code starts randomly asserting the same thing. Quod licet Iovi, non licet bovi. Linus
The pull request you sent on Mon, 13 May 2024 11:50:03 +0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.10-p1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/84c7d76b5ab6a52e1b3d8101b9f910c128dca396
Thank you!
On Mon, May 13, 2024 at 03:12:53PM -0700, Linus Torvalds wrote: > > Maybe somebody can teach the kernel build in *general* that > "kmalloc()" and friends never return an error pointer, only NULL or > success? That would not necessarily be a bad idea if the scope-based > cleanup otherwise causes issues. Yes he did try this out: https://lore.kernel.org/all/20240302082751.GA25828@wunner.de/ It resulted in an increase in total vmlinux size although I don't think anyone looked into the reason for it. > But this kind of hacky "one random piece of kernel code uses a very > dangerous pattern to state that some *other* piece of kernel code has > particular return patterns" - that is not at all acceptable. Agreed. However, this patch still has two outstanding build defects which have not been addressed: https://lore.kernel.org/all/202404240904.Qi3nM37B-lkp@intel.com/ https://lore.kernel.org/all/202404252210.KJE6Uw1h-lkp@intel.com/ So I might end up just reverting it. Thanks,
On Mon, 13 May 2024 at 22:17, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Yes he did try this out: > > https://lore.kernel.org/all/20240302082751.GA25828@wunner.de/ > > It resulted in an increase in total vmlinux size although I don't > think anyone looked into the reason for it. I think the basic issue is that the whole 'assume()' logic of "if (x) unreachable()" is very fragile. Basically, it *can* generate the exact code you want by basically telling the compiler that if some condition is true, then the compiler can jump to unreachable code, and then depending on the phase of the moon, the compiler may get the whole "I can assume this is never true". BUT. The reason I hated seeing it so much is exactly that it's basically depending on everything going just right. When things do *not* go right, it causes the compiler to instead actually generate the extra code for the conditional, and actually generate a conditional jump to something that the compiler then decides it can do anything to, since it's unreachable. So now you generate extra code, and generate a branch to nonsense. > However, this patch still has two outstanding build defects which > have not been addressed: > > https://lore.kernel.org/all/202404240904.Qi3nM37B-lkp@intel.com/ This one just seems to be a sanity check for "you shouldn't check kmalloc() for ERR_PTR", so it's a validation test that then doesn't like the new test in that 'assume()'. And the second one: > https://lore.kernel.org/all/202404252210.KJE6Uw1h-lkp@intel.com/ looks *very* much like the cases we've seen with clang in particular where clang goes "this code isn't reachable, so I'll just drop everything on the floor", and then it just becomes a fallthrough to whatever else code happens to come next. Most of the time that's just more (unrelated) code in the same function, but sometimes it causes that "falls through to next function" instead, entirely randomly depending on how the code was laid out. > So I might end up just reverting it. I suspect that because I removed the whole 'assume()' hackery, neither of the above issues will now happen, and the code nwo works. But yes, the above is *exactly* why I don't want to see that 'unreachable()' hackery. Now, if we had some *other* way to tell the compiler "this condition never happens", that would be fine. Some compiler builtin for asserting some condition. But a conditional "unreachable()" is absolutely not it. Linus
On Mon, May 13, 2024 at 10:41:58PM -0700, Linus Torvalds wrote: > > I suspect that because I removed the whole 'assume()' hackery, neither > of the above issues will now happen, and the code nwo works. Alright I'll let it stay and see if any new issues crop up. Thanks,
On Mon, 13 May 2024 at 23:54, Lukas Wunner <lukas@wunner.de> wrote: > > On Mon, May 13, 2024 at 03:12:53PM -0700, Linus Torvalds wrote: > > > > > https://lore.kernel.org/all/202404252210.KJE6Uw1h-lkp@intel.com/ > > > > looks *very* much like the cases we've seen with clang in particular > > where clang goes "this code isn't reachable, so I'll just drop > > everything on the floor", and then it just becomes a fallthrough to > > whatever else code happens to come next. Most of the time that's just > > more (unrelated) code in the same function, but sometimes it causes > > that "falls through to next function" instead, entirely randomly > > depending on how the code was laid out. > > Curiously, this particular 0-day report is for gcc 13.2.0 though, > not clang. Hmm. I think all the previous reports of "falls through to next function" that I have seen have been with clang, but that is probably be selection bias: the gcc cases of this tend to be found so much more quickly (because gcc is still more common at least on x86) that by the time I see the reports, it's because of some clang issue. And in fact, when I go test this theory by going to search on lore, I do see several gcc reports. So no, it was never just clang-only, it was just that the ones I had looked at were about clang. > The assume() macro had no effect with clang when I tested it. I suspect that the issue is that with *normal* kernel configurations, the code generation is simple and straightforward enough that gcc did the right thing. And then some more complicated setup with more debugging support enabled (particularly things like UBSAN or KASAN) the code gets complicated enough that gcc doesn't do the optimization any more, and then the conditional in assume() doesn't get optimized away at an early stage any more, and remains as a conditional branch to la-la-land. And you actually don't even see this as a warning unless the la-la-land happens to be at the end of a function. IOW, the "branch to nowhere" _could_ just branch to some label inside the function, and the objtool sanity check would never even have triggered. That's why "unreachable()" can be so dangerous. It tells the compiler that code generation in one place no longer matters, and then the compiler can decide to leave things just dangling in odd ways. The code presumably still *works* - because the actual conditional never triggers, so in that sense it's safe and fine. But it's still just horrendous to try to figure out, which is why I was so down on it. Linus
The pull request you sent on Fri, 19 Jul 2024 01:49:26 +1200:
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.11-p1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c434e25b62f8efcfbb6bf1f7ce55960206c1137e
Thank you!
The pull request you sent on Mon, 16 Sep 2024 11:59:01 +0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git v6.12-p1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/85ffc6e4ed3712f8b3fedb3fbe42afae644a699c
Thank you!