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-=wg4ETks+pGUco4gDrRxT+1UBbFGQtpOqSxLSzvVAWpm5w@mail.gmail.com>
Date: Tue, 30 Jul 2024 09:35:45 -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 03:11, Arnd Bergmann <arnd@...nel.org> wrote:
>
> I'm giving this a spin on the randconfig test setup now to see
> if there are some other cases like the bcachefs one. So far I've
> seen one failure, but I can't make sense of it yet:

So the new checks are actually a lot smarter, since unlike the old
ones they don't require a C constant expression, and will find cases
where the compiler can see expressions that turn out statically
optimizable.

This is a great example of that, although "great" in this case is
sadly not what we want:

> drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
> include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_905' declared with attribute error: clamp() low limit source_min greater than high limit source_max
> include/linux/minmax.h:107:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>   107 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
> drivers/gpu/drm/i915/display/intel_backlight.c:47:22: note: in expansion of macro 'clamp'
>    47 |         source_val = clamp(source_val, source_min, source_max);

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);
        }

and now the case with the WARN() will statically obviously be bad.

I don't see the failure, so it clearly depends on some config default,
and I suspect with my allmodconfig build, for example, there is so
much else going on that gcc won't have done the above trivial
conversion.

The old code never saw any of this, because the old code was using the
terminally stupid _static_assert(), and within the much more limited
scope of a "C constant expression", that "source_min < source_max"
could never be true, even if there are code paths where it *is* true.

But here I think we were bitten by excessive cleverness.

> That's still a typo in the 32-bit case, right?
> I've changed
>
>  __builtin_choose_expr(sizeof(ux)>32,1LL,1L))
>
> to check for sizeof(ux)>4 for my testing.

Bah yes. I had that fix locally, and sent the old patch.

            Linus