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.
