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-=whCvSUpbOawsbj4A6EUT7jO8562FG+vqiLQvW0CBBZZzA@mail.gmail.com>
Date: Mon, 29 Jul 2024 16:21:10 -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 Mon, 29 Jul 2024 at 15:25, Arnd Bergmann <arnd@...nel.org> wrote:
>
> - My macros use __builtin_choose_expr() instead of ?: to
>   ensure that the arguments are constant, this produces a
>   relatively clear compiler warning when they are not.
>   Without that, I would expect random drivers to start
>   using MIN()/MAX() in places where it's not safe.

Hmm. We have known non-constant uses, which Stephen Rothwell pointed
out elsewhere, with PowerPC having things like this:

  #define PAGE_SHIFT              CONFIG_PAGE_SHIFT
  extern unsigned int hpage_shift;
  #define HPAGE_SHIFT hpage_shift
  #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)

and honestly, considering the absolutely *horrid* mess that is the
current min/max expansion, I really think that using MIN/MAX for these
kinds of expressions is the right thing to do.

I don't worry about architecture code that has a boot-time
pseudo-constant for some inherent property.  I worry about random
drivers doing crazy things.

But it is *not* a constant, and any MIN/MAX that disallows it is imho
not a good MIN/MAX.

What we actually care about is not "constant", but "no side effects".

And obviously the typing, but that ends up being really hard.

We could possibly require that the typing be extra strict for MIN/MAX,
using the old __typecheck(x, y) macro we used to use. That literally
requires the *same* types, but that then ends up being pointlessly
painful for the "actual constant" cases, because dammit, a regular
non-negative integer constant should compare with any type.

It's a real pain that compiler writers can't just get the simple cases
right and give us a sane "__builtin_ordered_ok()" kind of thing.
Instead they push things like the absolute unusable garbage that is
-Wsign-compare.

Anyway, I also completely gave up on another absolute standard garbage
thing: _static_assert(). What a horrendous piece of sh*t that is.
Replacing our use of that with our own BUILD_BUG_ON_MSG() made a lot
of things much much simpler.

Attached is the patch I have in my tree right now - it complains about
a 'bcachefs' comparison between an 'u16' and a 's64', because I also
removed the 'implicit integer promotion is ok' logic, because I think
it's wrong.

I don't think a min(u16,s64) is a valid minimum, for exactly the same
reason a min(u32,s64) is not valid.

Despite the C integer expression promotion rules.

             Linus

View attachment "patch.diff" of type "text/x-patch" (2490 bytes)