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