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: <d48ce3b3-9173-4309-aae6-96be42327f97@lucifer.local>
Date: Fri, 26 Jul 2024 10:43:57 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Linus Torvalds <torvalds@...uxfoundation.org>
Cc: David Laight <David.Laight@...lab.com>, 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 Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote:
> On Thu, 25 Jul 2024 at 02:01, David Laight <David.Laight@...lab.com> wrote:
> >
> > The condition is '>= 0' so it doesn't matter if it is '1' or '0'.
>
> Yes, but that's because the whole conditional is so inexplicably complex.
>
> But the explanation is:
>
> > That gives a 'comparison of unsigned type against 0 is always true' warning.
> > (The compiler generates that for code in the unused branches of both
> > __builtin_choose_expr() and _Generic().)
> > Moving the comparison to the outer level stops all such compiler warnings.
>
> 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.

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?

>
> But it was also about a million times simpler, and didn't cause build
> time regressions.
>

:)

>              Linus