linux-kernel - Re: [GIT PULL] Crypto Update for 6.10

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wi1T6wq1USBfU=NjdpSaTiKzV4H2gnUQfKa_mcXqOSk_w@mail.gmail.com>
Date: Mon, 13 May 2024 22:41:58 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Lukas Wunner <lukas@...ner.de>, "David S. Miller" <davem@...emloft.net>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	Linux Crypto Mailing List <linux-crypto@...r.kernel.org>
Subject: Re: [GIT PULL] Crypto Update for 6.10

On Mon, 13 May 2024 at 22:17, Herbert Xu <herbert@...dor.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