linux-kernel - Re: [PATCH v21 09/24] ovpn: implement packet processing

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: <8abd4290-cef5-4ea3-bdfc-b872c16efb8a@openvpn.net>
Date: Wed, 5 Mar 2025 00:35:09 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, 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>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH v21 09/24] ovpn: implement packet processing

On 04/03/2025 20:02, Sabrina Dubroca wrote:
> 2025-03-04, 01:33:39 +0100, Antonio Quartulli wrote:
>> +struct crypto_aead *ovpn_aead_init(const char *title, const char *alg_name,
>> +				   const unsigned char *key,
>> +				   unsigned int keylen)
> 
> nit: static? I don't see it used outside this file.

ACK.

> 
> 
> [...]
>> +static inline struct ovpn_crypto_key_slot *
>> +ovpn_crypto_key_id_to_slot(const struct ovpn_crypto_state *cs, u8 key_id)
>> +{
>> +	struct ovpn_crypto_key_slot *ks;
>> +	u8 idx;
>> +
>> +	if (unlikely(!cs))
>> +		return NULL;
>> +
>> +	rcu_read_lock();
>> +	idx = cs->primary_idx;
> 
> I'd go with slots[0] and slots[1], since it doesn't really matter
> whether we check the primary or secondary first. It would avoid a
> possible reload of cs->primary_idx (which might be updated
> concurrently by a key swap and cause us to look into the same slot
> twice) -- a READ_ONCE would also prevent that.

Reason for looking into primary first is that we will most likely need 
the primary key to decrypt the incoming traffic.

Secondary is used only during a small (if at all) time window where we 
moved to a new key, but our peer was still sending traffic encrypted 
with the old (secondary) key.

Therefore optimizing for primary-first may make a non-negligible 
difference under heavy load.

Code doesn't get more complex due to this logic, therefore I'd keep this 
version (with READ_ONCE(cs->primary_idx)), unless there is a strong 
argument against it.


Thanks!

> 
>> +	ks = rcu_dereference(cs->slots[idx]);
>> +	if (ks && ks->key_id == key_id) {
>> +		if (unlikely(!ovpn_crypto_key_slot_hold(ks)))
>> +			ks = NULL;
>> +		goto out;
>> +	}
>> +
>> +	ks = rcu_dereference(cs->slots[!idx]);
>> +	if (ks && ks->key_id == key_id) {
>> +		if (unlikely(!ovpn_crypto_key_slot_hold(ks)))
>> +			ks = NULL;
>> +		goto out;
>> +	}
>> +
>> +	/* when both key slots are occupied but no matching key ID is found, ks
>> +	 * has to be reset to NULL to avoid carrying a stale pointer
>> +	 */
>> +	ks = NULL;
>> +out:
>> +	rcu_read_unlock();
>> +
>> +	return ks;
>> +}
> 

-- 
Antonio Quartulli
OpenVPN Inc.