linux-kernel - Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must

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: <66657ed6-b8ef-7e8d-b77c-9fd65453f53e@linaro.org>
Date:   Wed, 16 Sep 2020 16:33:49 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     davem@...emloft.net, vkoul@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bitfield.h: annotate type_replace_bits functions with
 __must_check



On 16/09/2020 16:20, Greg KH wrote:
> On Wed, Sep 16, 2020 at 04:03:33PM +0100, Srinivas Kandagatla wrote:
>> usage of apis like u32_replace_bits() without actually catching the return
>> value could hide problems without any warning!
>>
>> Found this with recent usage of this api in SoundWire!
>> Having __must_check annotation would really catch this issues in future!
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> ---
>>   include/linux/bitfield.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index 4e035aca6f7e..eb4f69253946 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -131,7 +131,7 @@ static __always_inline __##type type##_encode_bits(base v, base field)	\
>>   		__field_overflow();					\
>>   	return to((v & field_mask(field)) * field_multiplier(field));	\
>>   }									\
>> -static __always_inline __##type type##_replace_bits(__##type old,	\
>> +static __always_inline __must_check __##type type##_replace_bits(__##type old, \
>>   					base val, base field)		\
>>   {									\
>>   	return (old & ~to(field)) | type##_encode_bits(val, field);	\
>> -- 
>> 2.21.0
>>
> 
> Don't add __must_check to things that if merged will instantly cause
> build warnings to the system, that's just rude :(
Currently there are not many users for this api, found only one instance 
in drivers/net/ipa/ipa_table.c which is also fixed with 
https://lkml.org/lkml/2020/9/10/1062

> 
> Fix up everything first, and then try to make this type of change.
> 
> But why does this function have to be checked?
As this function would return updated value, this check is more to with 
using the return value!

It is easy for someone to ignore this return value assuming that the new 
value is already updated! So this check should help!

TBH, This is what happened when we(vkoul and me) tried use this api :-)

--srini

> 
> thanks,
> 
> greg k-h
>