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

Antonio Quartulli
2024-05-29T20:14:00Z

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.