linux-kernel - RE: [PATCH next v4 0/5] minmax: Relax type checks in min() and max().

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: <147531551ec54a558c91bbef47f035c8@AcuMS.aculab.com>
Date: Mon, 8 Jan 2024 13:34:36 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Jiri Slaby' <jirislaby@...il.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: Linus Torvalds <torvalds@...ux-foundation.org>, 'Andy Shevchenko'
	<andriy.shevchenko@...ux.intel.com>, 'Andrew Morton'
	<akpm@...ux-foundation.org>, "'Matthew Wilcox (Oracle)'"
	<willy@...radead.org>, 'Christoph Hellwig' <hch@...radead.org>, "'Jason A.
 Donenfeld'" <Jason@...c4.com>
Subject: RE: [PATCH next v4 0/5] minmax: Relax type checks in min() and max().

From: Jiri Slaby <jirislaby@...il.com>
> Sent: 08 January 2024 11:46
> 
> Hi,
> 
> On 18. 09. 23, 10:14, David Laight wrote:
> > The min() (etc) functions in minmax.h require that the arguments have
> > exactly the same types.
> >
> > However when the type check fails, rather than look at the types and
> > fix the type of a variable/constant, everyone seems to jump on min_t().
> > In reality min_t() ought to be rare - when something unusual is being
> > done, not normality.
> ...
> > David Laight (5):
> >    minmax: Add umin(a, b) and umax(a, b)
> >    minmax: Allow min()/max()/clamp() if the arguments have the same
> >      signedness.
> >    minmax: Fix indentation of __cmp_once() and __clamp_once()
> >    minmax: Allow comparisons of 'int' against 'unsigned char/short'.
> >    minmax: Relax check to allow comparison between unsigned arguments and
> >      signed constants.
> 
> This slows down the build and increases the build memory consumption so
> that it causes OOMs.
> 
> In particular 6.7:
> $ time make drivers/media/pci/solo6x10/solo6x10-p2m.i
> ...
>    CPP [M] drivers/media/pci/solo6x10/solo6x10-p2m.i
> real    0m45,002s
> user    0m40,840s
> sys     0m5,922s
> 
> 
> $ git revert 867046cc7027703f60a46339ffde91a1970f2901
> $ time make drivers/media/pci/solo6x10/solo6x10-p2m.i
> ...
>    CPP [M] drivers/media/pci/solo6x10/solo6x10-p2m.i
> real    0m11,132s
> user    0m9,737s
> sys     0m1,415s
> 
> 
> $ git revert 4ead534fba42fc4fd41163297528d2aa731cd121
> $ time make drivers/media/pci/solo6x10/solo6x10-p2m.i
> ...
>    CPP [M] drivers/media/pci/solo6x10/solo6x10-p2m.i
> real    0m3,711s
> user    0m3,041s
> sys     0m0,710s
> 
> 
> 
> Note it's only a preprocessor run. If you run a compiler on top of that,
> it even dies.
> 
> There is nothing special in that file, just:
> if (SOLO_SDRAM_END(solo_dev) > solo_dev->sdram_size) {
> 
> which at some point expands to
>          max(__SOLO_JPEG_MIN_SIZE(__solo),                               \
>              min((__solo->sdram_size - SOLO_JPEG_EXT_ADDR(__solo)),
> 0x00ff0000))
> 
> and that expands to a lot of stuff.
> 
> Note that _line_ is 519 kbytes on 6.6 already. And 6 MB on 6.7.

Even with trivial min() max() it is 7.5k (piped through xargs -s72):
(see https://godbolt.org/z/rzE39YodW)

The 'explosion' happens because there are nested max(a, min(b, c)).
I think: max(a, min(max(b, min(c, d)), e))

I actually can't help feeling that is the driver ever uses
any of these values they ought to be saved during initialisation.
That would likely save a lot of code later - as well as shrinking
this test to something sane.

return ((((((((0x00000000 + 0x00480000) + ((solo->type == 1 ?
0x10000 : 0x20000) * 32)) + 0x00080000) + 0x00010000) +
((((solo->sdram_size <= (32 << 20)) ? 4 : 16) + 1) * (18 << 16)))
+ (0x00140000 * solo->nr_chans * 2)) + (((solo->nr_chans *
0x00080000)) > (((((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) < (0x00ff0000) ?
(((solo->sdram_size - ((((((0x00000000 + 0x00480000) +
((solo->type == 1 ? 0x10000 : 0x20000) * 32)) + 0x00080000) +
0x00010000) + ((((solo->sdram_size <= (32 << 20)) ? 4 : 16) + 1) *
(18 << 16))) + (0x00140000 * solo->nr_chans * 2))) -
(solo->nr_chans * 0x00080000))) : (0x00ff0000))) ?
((solo->nr_chans * 0x00080000)) : (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))))) +
(((solo->nr_chans * 0x00080000)) > ((((solo->sdram_size -
(((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2)) + (((solo->nr_chans * 0x00080000)) >
(((((solo->sdram_size - ((((((0x00000000 + 0x00480000) +
((solo->type == 1 ? 0x10000 : 0x20000) * 32)) + 0x00080000) +
0x00010000) + ((((solo->sdram_size <= (32 << 20)) ? 4 : 16) + 1) *
(18 << 16))) + (0x00140000 * solo->nr_chans * 2))) -
(solo->nr_chans * 0x00080000))) < (0x00ff0000) ?
(((solo->sdram_size - ((((((0x00000000 + 0x00480000) +
((solo->type == 1 ? 0x10000 : 0x20000) * 32)) + 0x00080000) +
0x00010000) + ((((solo->sdram_size <= (32 << 20)) ? 4 : 16) + 1) *
(18 << 16))) + (0x00140000 * solo->nr_chans * 2))) -
(solo->nr_chans * 0x00080000))) : (0x00ff0000))) ?
((solo->nr_chans * 0x00080000)) : (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))))))) <
(0x00ff0000) ? ((solo->sdram_size - (((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)) +
(((solo->nr_chans * 0x00080000)) > (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))) ?
((solo->nr_chans * 0x00080000)) : (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))))))) :
(0x00ff0000))) ? ((solo->nr_chans * 0x00080000)) :
((((solo->sdram_size - (((((((0x00000000 + 0x00480000) +
((solo->type == 1 ? 0x10000 : 0x20000) * 32)) + 0x00080000) +
0x00010000) + ((((solo->sdram_size <= (32 << 20)) ? 4 : 16) + 1) *
(18 << 16))) + (0x00140000 * solo->nr_chans * 2)) +
(((solo->nr_chans * 0x00080000)) > (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))) ?
((solo->nr_chans * 0x00080000)) : (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))))))) <
(0x00ff0000) ? ((solo->sdram_size - (((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)) +
(((solo->nr_chans * 0x00080000)) > (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))) ?
((solo->nr_chans * 0x00080000)) : (((((solo->sdram_size -
((((((0x00000000 + 0x00480000) + ((solo->type == 1 ? 0x10000 :
0x20000) * 32)) + 0x00080000) + 0x00010000) + ((((solo->sdram_size
<= (32 << 20)) ? 4 : 16) + 1) * (18 << 16))) + (0x00140000 *
solo->nr_chans * 2))) - (solo->nr_chans * 0x00080000))) <
(0x00ff0000) ? (((solo->sdram_size - ((((((0x00000000 +
0x00480000) + ((solo->type == 1 ? 0x10000 : 0x20000) * 32)) +
0x00080000) + 0x00010000) + ((((solo->sdram_size <= (32 << 20)) ?
4 : 16) + 1) * (18 << 16))) + (0x00140000 * solo->nr_chans * 2)))
- (solo->nr_chans * 0x00080000))) : (0x00ff0000))))))) :
(0x00ff0000))))); }

	David

> 
> The file is 4.3M vs. 122M.
> 
> Could you investigate/fix/revert (at least) the above two commits?
> 
> thanks,
> --
> js

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)