| 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
| ||
|
Message-ID: <2025093026-gutter-avert-7f16@gregkh>
Date: Tue, 30 Sep 2025 16:25:28 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Eliav Farber <farbere@...zon.com>
Cc: sashal@...nel.org, mario.limonciello@....com, lijo.lazar@....com,
David.Laight@...lab.com, arnd@...nel.org,
linux-kernel@...r.kernel.org, stable@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.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?
thanks,
greg k-h
Powered by blists - more mailing lists