netdev - Re: [PATCH net-next v3 14/24] ovpn: implement multi-peer support

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]

Date: Wed, 29 May 2024 22:15:27 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
 Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 14/24] ovpn: implement multi-peer support

On 29/05/2024 17:16, Sabrina Dubroca wrote:
> 2024-05-28, 21:41:15 +0200, Antonio Quartulli wrote:
>> On 28/05/2024 16:44, Sabrina Dubroca wrote:
>>> Hi Antonio, I took a little break but I'm looking at your patches
>>> again now.
>>
>> Thanks Sabrina! Meanwhile I have been working on all your suggested changes.
>> Right now I am familiarizing with the strparser.
> 
> Cool :)
> 
>>> 2024-05-06, 03:16:27 +0200, Antonio Quartulli wrote:
>>>> +	index = ovpn_peer_index(ovpn->peers.by_id, &peer->id, sizeof(peer->id));
>>>> +	hlist_add_head_rcu(&peer->hash_entry_id, &ovpn->peers.by_id[index]);
>>>> +
>>>> +	if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
>>>> +		index = ovpn_peer_index(ovpn->peers.by_vpn_addr,
>>>> +					&peer->vpn_addrs.ipv4,
>>>> +					sizeof(peer->vpn_addrs.ipv4));
>>>> +		hlist_add_head_rcu(&peer->hash_entry_addr4,
>>>> +				   &ovpn->peers.by_vpn_addr[index]);
>>>> +	}
>>>> +
>>>> +	hlist_del_init_rcu(&peer->hash_entry_addr6);
>>>
>>> Why are hash_entry_transp_addr and hash_entry_addr6 getting a
>>> hlist_del_init_rcu() call, but not hash_entry_id and hash_entry_addr4?
>>
>> I think not calling del_init_rcu on hash_entry_addr4 was a mistake.
>>
>> Calling del_init_rcu on addr4, addr6 and transp_addr is needed to put them
>> in a known state in case they are not hashed.
> 
> hlist_del_init_rcu does nothing if node is not already on a list.

Mh you're right. I must have got confused for some reason.
Those del_init_rcu can go then.

> 
>> While hash_entry_id always goes through hlist_add_head_rcu, therefore
>> del_init_rcu is useless (to my understanding).
> 
> I'm probably missing something about how this all fits together. In
> patch 19, I see ovpn_nl_set_peer_doit can re-add a peer that is
> already added (but I'm not sure why, since you don't allow changing
> the addresses, so it won't actually be re-hashed).

Actually it's not a "re-add", but the intent is to "update" a peer that 
already exists. However, some fields are forbidden from being updated, 
like the address.

[NOTE: I found some issue with the "peer update" logic in 
ovpn_nl_set_peer_doit and it's being changed a bit]

> 
> I don't think doing a 2nd add of the same element to peers.by_id (or
> any of the other hashtables) is correct, so I'd say you need
> hlist_del_init_rcu for all of them.

This is exactly the bug I mentioned above: we should not go through the 
add again. Ideally we should just update the fields and be done with it, 
without re-hashing the object.

I hope it makes sense.

Cheers,

> 

-- 
Antonio Quartulli
OpenVPN Inc.