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: <16e1568d-8747-41e0-91b9-ce23c5592799@wanadoo.fr>
Date: Tue, 4 Feb 2025 00:41:55 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Johannes Berg <johannes@...solutions.net>,
 Yury Norov <yury.norov@...il.com>, 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 03/02/2025 at 22:59, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@...adoo.fr> wrote:
>> On 03/02/2025 at 16:44, Johannes Berg wrote:
>>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
>>>>> Instead of creating another variant for
>>>>> non-constant bitfields, wouldn't it be better to make the existing macro
>>>>> accept both?
>>>>
>>>> Yes, it would definitely be better IMO.
>>>
>>> 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.
> 
> Even that case looks valid to me. Actually there is already such a user
> in drivers/iio/temperature/mlx90614.c:
> 
>     ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);
> 
> So if you want enhanced safety, having both the safer/const upper-case
> variants and the less-safe/non-const lower-case variants makes sense.

So, we are scared of people calling FIELD_PREP() with the arguments in
the wrong order:

  FIELD_PREP(val, mask)

thus adding the check that mask must be a compile time constant.

But if we introduce a second function, don't we introduce the risk of
having people use the lower case variant instead of the upper case variant?

  field_prep(incorrect_const_mask, val)

I am not sure to follow the logic of why having two functions is the
safer choice. Whatever the solution you propose, there will be a way to
misuse it. Let me ask, what is the most likely to happen:

  1. wrong parameter order
  2. wrong function name

?

If you have the conviction that people more often do mistake 1. then I
am fine with your solution. Otherwise, if 1. and 2. have an equally
likelihood, then I would argue to go with the simplicity of the single
function.

Yours sincerely,
Vincent Mailhol