linux-kernel - Re: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

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-=whUjRyKL_+Zq2_wJNw4PaNT2uMW+eCg_PxPBmmAqSxLYA@mail.gmail.com>
Date: Tue, 30 Jul 2024 09:46:05 -0700
From: Linus Torvalds <torvalds@...uxfoundation.org>
To: Arnd Bergmann <arnd@...nel.org>
Cc: David Laight <David.Laight@...lab.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Jens Axboe <axboe@...nel.dk>, 
	Matthew Wilcox <willy@...radead.org>, Christoph Hellwig <hch@...radead.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Dan Carpenter <dan.carpenter@...aro.org>, "Jason A . Donenfeld" <Jason@...c4.com>, 
	"pedro.falcato@...il.com" <pedro.falcato@...il.com>, Mateusz Guzik <mjguzik@...il.com>, 
	"linux-mm@...ck.org" <linux-mm@...ck.org>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Subject: Re: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

On Tue, 30 Jul 2024 at 09:35, Linus Torvalds
<torvalds@...uxfoundation.org> wrote:
>
> So here *locally*, source_min and source_max can't be ordered, but
> what I think has happened is that we had that earlier
>
>         WARN_ON(source_min > source_max);
>
> and then gcc sees the "statically_true(ulo > uhi)" test, and will do
> CSE on the variables and on the test condition and the conditional,
> and basically have turned all of this into
>
>         if (source_min > source_max) {
>                 WARN(..)
>                 source_val = clamp(source_val, source_min, source_max);
>         } else {
>                 source_val = clamp(source_val, source_min, source_max);
>         }

Confirmed with your .config - removing the WARN_ON() removes the
clamping range error, because then there is no "move code into shared
conditional section" case any more.

That's slightly annoying. The new clamp() logic is not only a much
cleaner macro expansion, it's also *much* smarter and would find real
problems when the limits have been passed as arguments to inline
functions etc.

But obviously this "it's statically wrong in one path when the code
has been duplicated by the compiler" means that it's just too smart
for its own good in this case.

If the WARN_ON() had a "return error", it would all work out
beautifully. But now we literally have code that says "I just tested
for this error condition, and then I went ahead and did the error case
anyway", and that just makes my nice new sanity check unhappy.

Darn.

                Linus