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: <77c2a569-6f6c-41d2-ad85-2b0d71e9bae4@gmail.com>
Date: Tue, 12 Nov 2024 03:18:10 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Sabrina Dubroca <sd@...asysnail.net>,
 Antonio Quartulli <antonio@...nvpn.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 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