Re: [GIT PULL] Crypto Update for 6.10

From: Lukas Wunner <lukas@wunner.de>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	Julia Lawall <julia.lawall@inria.fr>,
	Nicolas Palix <nicolas.palix@imag.fr>,
	cocci@inria.fr
Subject: Re: [GIT PULL] Crypto Update for 6.10
Date: Tue, 14 May 2024 08:54:52 +0200	[thread overview]
Message-ID: <ZkMKvAnyOR3_cJnS@wunner.de> (raw)
In-Reply-To: <CAHk-=wi1T6wq1USBfU=NjdpSaTiKzV4H2gnUQfKa_mcXqOSk_w@mail.gmail.com> <CAHk-=wjmwmWv3sDCNq8c4VHWZUtZH72tDqR=TcgfpxTegL=aZw@mail.gmail.com>

On Mon, May 13, 2024 at 03:12:53PM -0700, Linus Torvalds wrote:
> 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
[...]
> 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.

Thanks, appreciated.  This way of handling it spares me from having
to resubmit the patch without assume().  (The patch is prep work
for upcoming PCI device authentication.)

> > 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()'.

I've been in touch with Julia (+cc) to silence this coccinelle
false-positive.  But now that the assume() is gone, the coccinelle
warning won't appear anyway:

https://lore.kernel.org/all/alpine.DEB.2.22.394.2405062136410.3284@hadrien/

> 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.

Curiously, this particular 0-day report is for gcc 13.2.0 though,
not clang.

The assume() macro had no effect with clang when I tested it.
So the unnecessary IS_ERR() check persisted despite the macro when
compiling with clang.  Only gcc honors it.  Probably another reason
why you would hate the macro. :)

clang supports __builtin_assume().  In theory that should have the
same effect as __builtin_unreachable() on gcc (albeit with inverse
boolean semantics).  In practice it had no effect.  (Tested with
clang 15.0.6.)

https://clang.llvm.org/docs/LanguageExtensions.html#builtin-assume

So with clang there doesn't seem to be a working way to tell the
compiler about assumptions it can make.  And with gcc it's apparently
"hit and miss" depending on the exact gcc version and code. :(

> I suspect that because I removed the whole 'assume()' hackery, neither
> of the above issues will now happen, and the code nwo works.

Yes.

I guess this effort was over the top, so apologies for the noise!

Thanks,

Lukas