netdev - Re: [PATCH net-next v11 14/23] 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: <52960609-2d3a-4e75-a31d-6643a0411435@openvpn.net>
Date: Tue, 12 Nov 2024 13:32:44 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Sabrina Dubroca <sd@...asysnail.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, Andrew Lunn <andrew@...n.ch>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 14/23] ovpn: implement peer lookup logic

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.