netdev - Re: [PATCH net-next v12 13/22] ovpn: implement peer lookup logic

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: <5ad48b64-1f83-4a62-addd-3008d5faa2f5@openvpn.net>
Date: Wed, 4 Dec 2024 15:13:14 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 ryazanov.s.a@...il.com, Simon Horman <horms@...nel.org>,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v12 13/22] ovpn: implement peer lookup logic

On 04/12/2024 09:28, Antonio Quartulli wrote:
> On 03/12/2024 17:09, Sabrina Dubroca wrote:
>> 2024-12-03, 15:58:17 +0100, Antonio Quartulli wrote:
>>> On 02/12/2024 16:07, Antonio Quartulli wrote:
>>> [...]
>>>> +#define ovpn_get_hash_slot(_key, _key_len, _tbl) ({    \
>>>> +    typeof(_tbl) *__tbl = &(_tbl);            \
>>>> +    jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl);    \
>>>> +})
>>>> +
>>>> +#define ovpn_get_hash_head(_tbl, _key, _key_len) ({        \
>>>> +    typeof(_tbl) *__tbl = &(_tbl);                \
>>>> +    &(*__tbl)[ovpn_get_hash_slot(_key, _key_len, *__tbl)];    \
>>>> +})
>>>
>>> clang a reporting various warnings like this:
>>>
>>> ../drivers/net/ovpn/peer.c:406:9: warning: variable '__tbl' is 
>>> uninitialized
>>> when used within its own initialization [-Wuninitialized]
>>>    406 |         head = ovpn_get_hash_head(ovpn->peers->by_id, &peer_id,
>>>        |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    407 |                                   sizeof(peer_id));
>>>        |                                   ~~~~~~~~~~~~~~~~
>>> ../drivers/net/ovpn/peer.c:179:48: note: expanded from macro
>>> 'ovpn_get_hash_head'
>>>    179 |         &(*__tbl)[ovpn_get_hash_slot(_key, _key_len, 
>>> *__tbl)];  \
>>>        |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
>>> ../drivers/net/ovpn/peer.c:173:26: note: expanded from macro
>>> 'ovpn_get_hash_slot'
>>>    173 |         typeof(_tbl) *__tbl = &(_tbl);                  \
>>>        |                       ~~~~~     ^~~~
>>>
>>> Anybody willing to help me understand this issue?
>>>
>>> I have troubles figuring out how __tbl is being used uninitialized.
>>> I wonder if the parameters naming is fooling clang (or me) somehow.
>>
>> Not really a solution to this specific issue, but do you actually need
>> ovpn_get_hash_slot as a separate macro? AFAICT all users could also be
>> converted to ovpn_get_hash_head, then you can merge ovpn_get_hash_slot
>> into ovpn_get_hash_head and maybe clang won't get confused?
>>
>> No guarantee that this fixes anything (except saving one or two lines
>> in a few functions).
> 
> This is what it used to be before (and no error was reported), but I had 
> to split the macro because I need to isolate the slot computation for 
> nulls comparison. So there are some users for ovpn_get_hash_slot()
> 
> I will quickly try changing the naming and see if clang gets happier.

Indeed it's the declaration of __tbl in ovpn_get_hash_slot() that 
confuses clang.
I'll rename both __tbl and add a comment to remember why we did that.

Regards,

> 
> Regards,
> 
> 

-- 
Antonio Quartulli
OpenVPN Inc.