linux-kernel - RE: [PATCH v2 03/13 6.1.y] minmax: simplify min()/max()/clamp() implementation

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] [day] [month] [year] [list]
Message-ID: <705cb94d16234e44b9bfd33b6e87471d@amazon.com>
Date: Tue, 30 Sep 2025 14:49:07 +0000
From: "Farber, Eliav" <farbere@...zon.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: "sashal@...nel.org" <sashal@...nel.org>, "mario.limonciello@....com"
	<mario.limonciello@....com>, "lijo.lazar@....com" <lijo.lazar@....com>,
	"David.Laight@...lab.com" <David.Laight@...lab.com>, "arnd@...nel.org"
	<arnd@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "stable@...r.kernel.org"
	<stable@...r.kernel.org>, Linus Torvalds <torvalds@...ux-foundation.org>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, "Farber, Eliav"
	<farbere@...zon.com>
Subject: RE: [PATCH v2 03/13 6.1.y] minmax: simplify min()/max()/clamp()
 implementation

> On Mon, Sep 29, 2025 at 06:33:48PM +0000, Eliav Farber wrote:
> > From: Linus Torvalds <torvalds@...ux-foundation.org>
> >
> > [ Upstream commit dc1c8034e31b14a2e5e212104ec508aec44ce1b9 ]
> >
> > Now that we no longer have any C constant expression contexts (ie array
> > size declarations or static initializers) that use min() or max(), we
> > can simpify the implementation by not having to worry about the result
> > staying as a C constant expression.
> >
> > So now we can unconditionally just use temporary variables of the right
> > type, and get rid of the excessive expansion that used to come from the
> > use of
> >
> >    __builtin_choose_expr(__is_constexpr(...), ..
> >
> > to pick the specialized code for constant expressions.
> >
> > Another expansion simplification is to pass the temporary variables (in
> > addition to the original expression) to our __types_ok() macro.  That
> > may superficially look like it complicates the macro, but when we only
> > want the type of the expression, expanding the temporary variable names
> > is much simpler and smaller than expanding the potentially complicated
> > original expression.
> >
> > As a result, on my machine, doing a
> >
> >   $ time make drivers/staging/media/atomisp/pci/isp/kernels/ynr/ynr_1.0/ia_css_ynr.host.i
> >
> > goes from
> >
> >       real    0m16.621s
> >       user    0m15.360s
> >       sys     0m1.221s
> >
> > to
> >
> >       real    0m2.532s
> >       user    0m2.091s
> >       sys     0m0.452s
> >
> > because the token expansion goes down dramatically.
> >
> > In particular, the longest line expansion (which was line 71 of that
> > 'ia_css_ynr.host.c' file) shrinks from 23,338kB (yes, 23MB for one
> > single line) to "just" 1,444kB (now "only" 1.4MB).
> >
> > And yes, that line is still the line from hell, because it's doing
> > multiple levels of "min()/max()" expansion thanks to some of them being
> > hidden inside the uDIGIT_FITTING() macro.
> >
> > Lorenzo has a nice cleanup patch that makes that driver use inline
> > functions instead of macros for sDIGIT_FITTING() and uDIGIT_FITTING(),
> > which will fix that line once and for all, but the 16-fold reduction in
> > this case does show why we need to simplify these helpers.
> >
> > Cc: David Laight <David.Laight@...lab.com>
> > Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > Signed-off-by: Eliav Farber <farbere@...zon.com>
> > ---
> >  include/linux/minmax.h | 43 ++++++++++++++++++++----------------------
> >  1 file changed, 20 insertions(+), 23 deletions(-)
>
> This change breaks the build in drivers/md/ :
>
> In file included from ./include/linux/container_of.h:5,
>                  from ./include/linux/list.h:5,
>                  from ./include/linux/wait.h:7,
>                  from ./include/linux/mempool.h:8,
>                  from ./include/linux/bio.h:8,
>                  from drivers/md/dm-bio-record.h:10,
>                  from drivers/md/dm-integrity.c:9:
> drivers/md/dm-integrity.c: In function ‘integrity_metadata’:
> drivers/md/dm-integrity.c:131:105: error: ISO C90 forbids variable length array ‘checksums_onstack’ [-Werror=vla]
>   131 | #define MAX_TAG_SIZE                    (JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR - offsetof(struct journal_entry, last_bytes[MAX_SECTORS_PER_BLOCK]))
>       |                                                                                                         ^~~~~~~~~~~~~
>./include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’
>    78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
>       |                                                        ^~~~
>./include/linux/minmax.h:56:9: note: in expansion of macro ‘static_assert’
>    56 |         static_assert(__types_ok(x, y, ux, uy),         \
>       |         ^~~~~~~~~~~~~
>./include/linux/minmax.h:41:31: note: in expansion of macro ‘__is_noneg_int’
>    41 |          __is_noneg_int(x) || __is_noneg_int(y))
>       |                               ^~~~~~~~~~~~~~
>./include/linux/minmax.h:56:23: note: in expansion of macro ‘__types_ok’
>    56 |         static_assert(__types_ok(x, y, ux, uy),         \
>       |                       ^~~~~~~~~~
>./include/linux/minmax.h:61:9: note: in expansion of macro ‘__careful_cmp_once’
>    61 |         __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>       |         ^~~~~~~~~~~~~~~~~~
>./include/linux/minmax.h:92:25: note: in expansion of macro ‘__careful_cmp’
>    92 | #define max(x, y)       __careful_cmp(max, x, y)
>       |                         ^~~~~~~~~~~~~
> drivers/md/dm-integrity.c:1797:40: note: in expansion of macro ‘max’
>  1797 |                 char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
>       |                                        ^~~
> drivers/md/dm-integrity.c:131:89: note: in expansion of macro ‘offsetof’
>   131 | #define MAX_TAG_SIZE                    (JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR - offsetof(struct journal_entry, last_bytes[MAX_SECTORS_PER_BLOCK]))
>       |                                                                                         ^~~~~~~~
> drivers/md/dm-integrity.c:1797:73: note: in expansion of macro ‘MAX_TAG_SIZE’
>  1797 |                 char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
>       |                                                                         ^~~~~~~~~~~~
>
>
> So I'll stop here on this series.
>
> After the next release, can you rebase the series and resend the remaining ones after they are fixed up to build properly?

Sure.
I see the problem.
In the integrity_metadata() function it should be:
  char checksums_onstack[MAX(HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
instead of:
  char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];

---
Regards, Eliav