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

[Antonio Quartulli <antonio@openvpn.net>]

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.