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

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

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).
> 
>> +
>> +	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?

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?

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

--
Sergey