linux-kernel - Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field

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: <2904baea9188a4707d4b5a9a6bfa517a54323f8a.camel@sipsolutions.net>
Date: Mon, 03 Feb 2025 16:31:36 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>, Yury Norov
	 <yury.norov@...il.com>
Cc: Geert Uytterhoeven <geert+renesas@...der.be>, linux-clk@...r.kernel.org,
 	linux-arm-kernel@...ts.infradead.org, linux-renesas-soc@...r.kernel.org, 
	linux-crypto@...r.kernel.org, qat-linux@...el.com,
 linux-gpio@...r.kernel.org, 	linux-aspeed@...ts.ozlabs.org,
 linux-iio@...r.kernel.org, 	linux-sound@...r.kernel.org,
 linux-kernel@...r.kernel.org, Michael Turquette	 <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>, Nicolas Ferre	
 <nicolas.ferre@...rochip.com>, Alexandre Belloni
 <alexandre.belloni@...tlin.com>,  Claudiu Beznea
 <claudiu.beznea@...on.dev>, Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
 Herbert Xu	 <herbert@...dor.apana.org.au>, "David S . Miller"
 <davem@...emloft.net>,  Linus Walleij <linus.walleij@...aro.org>, Bartosz
 Golaszewski <brgl@...ev.pl>, Joel Stanley <joel@....id.au>,  Andrew Jeffery
 <andrew@...econstruct.com.au>, Crt Mori <cmo@...exis.com>, Jonathan Cameron
 <jic23@...nel.org>,  Lars-Peter Clausen	 <lars@...afoo.de>, Jacky Huang
 <ychuang3@...oton.com>, Shan-Chun Hung	 <schung@...oton.com>, Rasmus
 Villemoes <linux@...musvillemoes.dk>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, Jakub Kicinski <kuba@...nel.org>, Alex Elder
 <elder@...e.org>
Subject: Re: [PATCH treewide v2 1/3] bitfield: Add non-constant
 field_{prep,get}() helpers

On Mon, 2025-02-03 at 22:36 +0900, Vincent Mailhol wrote:
> > On the flip side, there have been discussions in the past (though I
> > think not all, if any, on the list(s)) about the argument order. Since
> > the value is typically not a constant, requiring the mask to be a
> > constant has ensured that the argument order isn't as easily mixed up as
> > otherwise.
> 
> If this is a concern, then it can be checked with:
> 
>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
>                    __builtin_constant_p(_val),
>                    _pfx "mask is not constant");
> 
> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> any other combination.

There almost certainly will be users who want both to be non-constant
though, and anyway I don't understand how that helps - if you want to
write the value 0x7 to the (variable) mask 0xF then this won't catch
anything?

> > However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost
> > certainly shouldn't be done for the same reason - not compiling for non-
> > constant values is [IMHO] part of the API contract for that macro. This
> > can be important for the same reasons.
> 
> Your point is fair enough. But I do not see this as a killer argument.
> We can instead just add below helper:
> 
>   BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2()
> 
> But, for the same reason why I would rather not have both the
> FIELD_{PREP,GET}() and the field_{prep,get}(), I would also rather not
> have a BUILD_BUG_ON_NOT_POWER_OF_2() and a
> BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2().
> 
> If your concern is the wording of the contract, the description can just
> be updated.

No, I just think in both cases it's really bad form to silently update
the contract removing negative assertions that other people may have
been relying on. Not because these trigger today, of course, but because
they may not have added additional checks, or similar.

So arguably then you should have BUILD_BUG_ON_CONST_NOT_POWER_OF_2() or
so instead, so that all existing users are unaffected by the updates,
and similarly that's an argument for leaving FIELD_* versions intact. Or
I guess one could change all existing users to new ones accordingly, say
FIELD_*_CONST_MASK(), but that's pretty annoying too.

johannes