linux-kernel - Re: [PATCH 1/1] kbuild: Only enable -Wtautological-constant-out-of-range-compare for W=2

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: <8b6d335b-d473-4442-a17f-497ae7996165@app.fastmail.com>
Date: Mon, 22 Dec 2025 11:20:18 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "David Laight" <david.laight.linux@...il.com>,
 "Linus Torvalds" <torvalds@...ux-foundation.org>
Cc: "Nathan Chancellor" <nathan@...nel.org>,
 "Nicolas Schier" <nsc@...nel.org>, linux-kbuild@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] kbuild: Only enable
 -Wtautological-constant-out-of-range-compare for W=2

On Sat, Dec 20, 2025, at 13:15, David Laight wrote:
> On Sat, 20 Dec 2025 11:27:13 +0100
>> 
>> This does seem like a completely sensible warning to me, and it's
>> always been enabled by default. I see three patches in the git history
>> (all from Nathan), which all make sense as well.
>> 
>> > Inside FIELD_PREP_CONST(mask, val) there is (with the patch, and if I've
>> > typed it correctly):
>> > 	BUILD_BUG_ON_ZERO(!(mask) || (mask) & ((mask) + ((mask) & -(mask)))))
>> > to check the mask is non-zero and contiguous bits.  
>> 
>> I think the problem is (as so often) the linux/bitfield.h headers
>> making things way too complicated. That condition makes no sense to
>> me, and neither would I expect a compiler to make sense of it either.
>
> It is simple really :-)
> -mask is (~mask + 1) so its lowest set bit is the same at that of mask.
> Adding mask changes the adjacent 1s to zeros.
> Anding with mask is then any high bits that are the same in both.
> So is non-zero if mask has noncontiguous bits in it.

The bit that I find most confusing here is how you have a boolean '||'
operation of two integers, but then interpret the result as an
integer again.

> Adding ' == 0' and ' != 0' would just make the line longer.

I don't think we care about the link length here at all.
Splitting it up into two BUILD_BUG_ON() or BUILD_BUG_ON_ZERO()
lines would help here as well.

>> If there is no way to express those conditions more clearly, I would
>> prefer removing the BUILD_BUG_ON stuff from the bitfield.h header,
>> it keeps causing way more false positives than finding actual bugs
>> with the input.
>
> I was just trying to reduce the .i lines line from 18KB for a typical use.

That seems like a worthwhile goal, but I think the only way to
make an actual impact here is to reduce the fan-out and evaluate
'mask' less than the current five times in that line (plus additional
evalations. If the 'mask' value is defined using complex macros
like ilog2() or max3() already, the expansion explodes.

Unfortunately the constant version of these macros can't use
compound statements, otherwise we could use an __auto_type temporary
here.

> Probably the only useful check is statically_true(hi < lo) in GENMASK.

Agreed, that one is clearly worth keeping.

    Arnd