linux-kernel - Re: [PATCH v2] driver core: Explicitly initialize struct member @data.have_async in __device

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7a464c15-d42e-408f-9b7e-55263299f1e1@icloud.com>
Date: Sat, 24 Aug 2024 05:12:03 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org,
 Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v2] driver core: Explicitly initialize struct member
 @data.have_async in __device_attach()

On 2024/8/23 23:55, Dmitry Torokhov wrote:
> On Fri, Aug 23, 2024 at 08:00:14PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>
>> __device_attach() relies on compiler to implicitly initialize struct
>> member @data.have_async to avoid the member is used before initialization
>> but readers may not understand that, solved by explicitly initializing
>> @data.have_async as well as existing @data.want_async.
> 
> I do not believe this is needed. We require kernel developers be
> familiar with the language of choice for the kernel.
> 
> We have a ton of partial or empty structure initializers in the kernel.
> If we count only empty non-static ones I see:
> 
> dtor@...r-ws:~/kernel/work $ git grep '= \?{ \?};' | grep -v static | wc -l
> 5707
> 
> Rough count of partial initializers is (might be over eager and some
> of them could be full ones, on the other hand it does not count
> initializers that span multiple lines and start with opening brace
> only):
> 
> dtor@...r-ws:~/kernel/work $ git grep '= \?{ \..* };' | grep -v static | wc -l
> 1150
> 
> Are you planning to go through all of them and add complete
> initializers? And keep adjusting them when structures will get extended?
> For what gain?
> 
actually, many partial initializers is proper, for example, we don't use
these fields that are implicitly initialized, for example, another usage
of the same same in the file drivers/base/dd.c.

no, let me take time to check if there are other usages in kernel tree
similar the one we discuss.

> There was no readers confusion, you wrote a tool for C static analysis
> that did not follow C standard and gave you a false warning. Please fix
> your tool instead.
> 
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>> Changes in v2:
>> - Remove both fix and stable tag
>> - Correct both title and commit messages
>> - Link to v1: https://lore.kernel.org/r/20240823-fix_have_async-v1-1-43a354b6614b@quicinc.com
>> ---
>>  drivers/base/dd.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9b745ba54de1..b0c44b0846aa 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -1021,6 +1021,7 @@ static int __device_attach(struct device *dev, bool allow_async)
>>  			.dev = dev,
>>  			.check_async = allow_async,
>>  			.want_async = false,
>> +			.have_async = false,
>>  		};
>>  
>>  		if (dev->parent)
>>
>> ---
>> base-commit: 87ee9981d1f86ee9b1623a46c7f9e4ac24461fe4
>> change-id: 20240823-fix_have_async-3a135618d91b
>>
>> Best regards,
>> -- 
>> Zijun Hu <quic_zijuhu@...cinc.com>
>>
> 
> Thanks.
>