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

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.

[...]

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

• 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;

+}