linux-kernel - Re: [PATCH v2] bitfield: Add 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] [day] [month] [year] [list]

Message-ID: <d5fee986-15c6-44ae-b08b-82e91445e922@quicinc.com>
Date: Fri, 11 Apr 2025 18:43:26 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Yury Norov <yury.norov@...il.com>
CC: <linux@...musvillemoes.dk>, <linux-kernel@...r.kernel.org>,
        <andrew@...n.ch>, <quic_kkumarcs@...cinc.com>,
        <quic_linchen@...cinc.com>, <quic_leiwei@...cinc.com>,
        <quic_suruchia@...cinc.com>, <quic_pavir@...cinc.com>
Subject: Re: [PATCH v2] bitfield: Add FIELD_MODIFY() helper



On 4/11/2025 12:18 AM, Yury Norov wrote:
> On Thu, Apr 10, 2025 at 09:10:48PM +0800, Luo Jie wrote:
>> Add a helper for replacing the contents of bitfield in memory
>> with the specified value.
>>
>> Even though a helper xxx_replace_bits() is available, it is not
>> well documented, and only reports errors at the run time, which
>> will not be helpful to catch possible overflow errors due to
>> incorrect parameter types used.
>>
>> Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
>> macros. It is functionally similar as xxx_replace_bits(), and in
>> addition adds the compile time type checking.
>>
>> FIELD_MODIFY(&reg, REG_FIELD_C, c) is the wrapper of the code below.
>> reg &= ~REG_FIELD_C;
>> reg |= FIELD_PREP(REG_FIELD_C, c);
>>
>> Signed-off-by: Luo Jie <quic_luoj@...cinc.com>
>> ---
>>
>> The new added macro FIELD_MODIFY() is expected to be used by the
>> following Ethernet PPE driver as link.
>> https://lore.kernel.org/linux-arm-msm/20250209-qcom_ipq_ppe-v3-0-453ea18d3271@quicinc.com/
>>
>> Changes in v2:
>> - Update the documented example for FIELD_MODIFY().
>> - Improve the commit message to describe the need for the change.
>>
>>   include/linux/bitfield.h | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index 63928f173223..421c7701a18d 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -7,8 +7,9 @@
>>   #ifndef _LINUX_BITFIELD_H
>>   #define _LINUX_BITFIELD_H
>>   
>> -#include <linux/build_bug.h>
>>   #include <asm/byteorder.h>
>> +#include <linux/build_bug.h>
>> +#include <linux/typecheck.h>
> 
> Don't change the headers order: linux first, asm next.

Understand, I will fix this order in the next version.

> 
>>   
>>   /*
>>    * Bitfield access macros
>> @@ -38,8 +39,7 @@
>>    *	  FIELD_PREP(REG_FIELD_D, 0x40);
>>    *
>>    * Modify:
>> - *  reg &= ~REG_FIELD_C;
>> - *  reg |= FIELD_PREP(REG_FIELD_C, c);
>> + *  FIELD_MODIFY(REG_FIELD_C, &reg, c);
>>    */
>>   
>>   #define __bf_shf(x) (__builtin_ffsll(x) - 1)
>> @@ -156,6 +156,23 @@
>>   		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
>>   	})
>>   
>> +/**
>> + * FIELD_MODIFY() - modify a bitfield element
>> + * @_mask: shifted mask defining the field's length and position
>> + * @_reg_p: pointer to the memory that should be updated
>> + * @_val: value to store in the bitfield
>> + *
>> + * FIELD_MODIFY() modifies the set of bits in @_reg_p specified by @_mask,
>> + * by replacing them with the bitfield value passed in as @_val.
>> + */
> 
> Please inspect the codebase and convert existing opencoded FIELD_MODIFY()s.
> I don't ask you to convert every driver out there, but core kernel files
> should be clear.
> 
> The first good candidate for you is __tlbi_level() in arm64. You may want
> to use Coccinelle to automate the search.
> 
> Thanks,
> Yury

OK, thanks for the suggestions. I will use the tool to find and convert
the opencoded FIELD_MODIFY() instances.

> 
>> +#define FIELD_MODIFY(_mask, _reg_p, _val)				\
>> +	({								\
>> +		typecheck_pointer(_reg_p);				\
>> +		__BF_FIELD_CHECK(_mask, *(_reg_p), _val, "FIELD_MODIFY: "); \
>> +		*(_reg_p) &= ~(_mask);					\
>> +		*(_reg_p) |= ((_val) << __bf_shf(_mask)) & (_mask);	\
>> +	})
>> +
>>   extern void __compiletime_error("value doesn't fit into mask")
>>   __field_overflow(void);
>>   extern void __compiletime_error("bad bitfield mask")
>> -- 
>> 2.34.1