Re: [PATCH net-next v11 14/23] ovpn: implement peer lookup logic

[Antonio Quartulli <antonio@openvpn.net>]

On 12/11/2024 02:18, Sergey Ryazanov wrote:
> On 04.11.2024 13:26, Sabrina Dubroca wrote:
>> 2024-10-29, 11:47:27 +0100, Antonio Quartulli wrote:
>>>   struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct 
>>> *ovpn,
>>>                              struct sk_buff *skb)
>>>   {
>>> -    struct ovpn_peer *peer = NULL;
>>> +    struct ovpn_peer *tmp, *peer = NULL;
>>>       struct sockaddr_storage ss = { 0 };
>>> +    struct hlist_nulls_head *nhead;
>>> +    struct hlist_nulls_node *ntmp;
>>> +    size_t sa_len;
>>>       if (unlikely(!ovpn_peer_skb_to_sockaddr(skb, &ss)))
>>>           return NULL;
>>>       if (ovpn->mode == OVPN_MODE_P2P)
>>> -        peer = ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss);
>>> +        return ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss);
>>> +
>>> +    switch (ss.ss_family) {
>>> +    case AF_INET:
>>> +        sa_len = sizeof(struct sockaddr_in);
>>> +        break;
>>> +    case AF_INET6:
>>> +        sa_len = sizeof(struct sockaddr_in6);
>>> +        break;
>>> +    default:
>>> +        return NULL;
>>> +    }
>>
>> You could get rid of that switch by having ovpn_peer_skb_to_sockaddr
>> also set sa_len (or return 0/the size).

Yeah, makes sense. Thanks!

>>
>>> +
>>> +    nhead = ovpn_get_hash_head(ovpn->peers->by_transp_addr, &ss, 
>>> sa_len);
>>> +
>>> +    rcu_read_lock();
>>> +    hlist_nulls_for_each_entry_rcu(tmp, ntmp, nhead,
>>> +                       hash_entry_transp_addr) {
>>
>> I think that's missing the retry in case we ended up in the wrong
>> bucket due to a peer rehash?

Oh, for some reason I convinced myself that this is handled behind the 
scene, but indeed the lookup must be explicitly restarted.

will fix it, thanks for pointing this out!


> 
> Nice catch! I am also wondering why the 'nulls' variant was selected, 
> but there are no nulls value verification with the search respin.
> 
> Since we started discussing the list API, why the 'nulls' variant is 
> used for address hash tables and the normal variant is used for the 
> peer-id lookup?

Because the nulls variant is used only for tables where a re-hash can 
happen.

The peer-id table does not expect its objected to be re-used or 
re-hashed since the ID of a peer cannot change throughout its lifetime.


Regards,


> 
>>
>>> +        if (!ovpn_peer_transp_match(tmp, &ss))
>>> +            continue;
>>> +
>>> +        if (!ovpn_peer_hold(tmp))
>>> +            continue;
>>> +
>>> +        peer = tmp;
>>> +        break;
>>> +    }
>>> +    rcu_read_unlock();
>>>       return peer;
>>>   }
> 
> -- 
> Sergey
> 

-- 
Antonio Quartulli
OpenVPN Inc.