linux-kernel - Re: [PATCH 4/7] minmax: Simplify signedness check

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: <f1601a08-6d6e-4074-8f86-4be7869641d8@lucifer.local>
Date: Fri, 26 Jul 2024 14:27:06 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Laight <David.Laight@...lab.com>
Cc: Linus Torvalds <torvalds@...uxfoundation.org>,
        Arnd Bergmann <arnd@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        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>
Subject: Re: [PATCH 4/7] minmax: Simplify signedness check

On Fri, Jul 26, 2024 at 12:57:43PM GMT, David Laight wrote:
> From: Lorenzo Stoakes
> > Sent: 26 July 2024 10:44
> >
> > On Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote:

[snip]

> > > Christ. This whole series is a nightmare of "add complexity to deal
> > > with stupid issues".
> > >
> > > But the kernel test robot clearly found even more issues.
> > >
> > > I think we need to just go back to the old code. It was stupid and
> > > limited and caused us to have to be more careful about types than was
> > > strictly necessary.
> >
> > The problem is simply reverting reveals that seemingly a _ton_ of code has
> > come to rely on the relaxed conditions.
> >
> > When I went to gather the numbers for my initial report I had to manually
> > fix up every case which was rather painful, and that was just a defconfig +
> > a few extra options. allmodconfig is likely to be a hellscape.
> >
> > I've not dug deep into the ins + outs of this, so forgive me for being
> > vague (Arnd has a far clearer understanding) - but it seems that the
> > majority of the complexity comes from having to absolutely ensure all this
> > works for compile-time constant values.
>
> The problems arise due to some odd uses, not just the requirement for compile-time
> constants for on-stack array sizes.

Odd implies not many, same argument applies.

[snip]

>
> > Arnd had a look through and determined there weren't _too_ many cases where
> > we need this (for instance array sizes).
> >
> > So I wonder whether we can't just vastly simplify this stuff (and reducing
> > the macro expansion hell) for the usual case, and implement something like
> > cmin()/cmax() or whatever for the true-constant cases?
>
> I did do that in a patch set from much earlier in the year.
> But Linus said they'd need to be MIN() and MAX() and that requires changes
> to a few places where those are already defined.

OK, so what's stopping you from doing that?

In order to implement a MIN()/MAX() you'd need to change call sites only
(should be a managable amount), so we can change this too?

I'm concerned that a solution is being proposed here and then handwaved
away...

Unfortunately a revert is no longer possible (I had to fix up 33 call sites
manually just for my defconfig build to compare perf before/after), so if
the intent is to eliminate the complexity, then we need a practical way
forward.

>
> > > But it was also about a million times simpler, and didn't cause build
> > > time regressions.
>
> Just bugs because people did min_t(short, 65536, 128) and didn't expect zero.
>
> It has to be said that the chances of a min(negative_value, unsigned_constant)
> appearing are pretty slim.
> All these tests are there to trap that case.
>
> There is always the option of disabling the tests for 'normal' build, but
> leaving them there for (say) the W=1 builds.
> Then it won't matter as much if the tests slow down the build a little.

Very much NAK disabling tests as a solution! Also leaving them for a build
that's apparently broken... yeah not a fan.

>
> I think I have tried a W=1 build - but there are too many warnings/errors
> from other places to get anywhere.
> A lot are for 'unsigned_var >= 0' in paths that get optimised away.
> The compiler doesn't help!
>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>