Re: [PATCH v21 09/24] ovpn: implement packet processing
2025-03-05, 00:35:09 +0100, Antonio Quartulli wrote:
> On 04/03/2025 20:02, Sabrina Dubroca wrote:
> > 2025-03-04, 01:33:39 +0100, Antonio Quartulli wrote:
> > [...]
> > > +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.
Ok, sounds reasonable.
--
Sabrina
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.
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;
> +}
--
Sabrina
[PATCH v21 09/24] ovpn: implement packet processing
This change implements encryption/decryption and
encapsulation/decapsulation of OpenVPN packets.
Support for generic crypto state is added along with
a wrapper for the AEAD crypto kernel API.
Signed-off-by: Antonio Quartulli
---
drivers/net/Kconfig| 4 +
drivers/net/ovpn/Makefile | 3 +
drivers/net/ovpn/bind.c| 9 +-
drivers/net/ovpn/crypto.c | 152
drivers/net/ovpn/crypto.h | 139 +++
drivers/net/ovpn/crypto_aead.c | 391 +
drivers/net/ovpn/crypto_aead.h | 31
drivers/net/ovpn/io.c | 156 ++--
drivers/net/ovpn/io.h | 3 +
drivers/net/ovpn/peer.c| 29 +++
drivers/net/ovpn/peer.h| 5 +
drivers/net/ovpn/pktid.c | 129 ++
drivers/net/ovpn/pktid.h | 87 +
drivers/net/ovpn/proto.h | 32
drivers/net/ovpn/skb.h | 5 +
15 files changed, 1158 insertions(+), 17 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index
b18ff941944e2e92aa769d1ebbc3d1782611fc06..51d77f3c0848c3c9425b586c6a90cff99a744390
100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -121,6 +121,10 @@ config OVPN
depends on IPV6 || !IPV6
select DST_CACHE
select NET_UDP_TUNNEL
+ select CRYPTO
+ select CRYPTO_AES
+ select CRYPTO_GCM
+ select CRYPTO_CHACHA20POLY1305
help
This module enhances the performance of the OpenVPN userspace software
by offloading the data channel processing to kernelspace.
diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile
index
164f2058ea8e6dc5b9287afb59758a268b2f8b56..38c9fdca0e2e8e4af3c369ceb3971b58ab52d77b
100644
--- a/drivers/net/ovpn/Makefile
+++ b/drivers/net/ovpn/Makefile
@@ -8,10 +8,13 @@
obj-$(CONFIG_OVPN) := ovpn.o
ovpn-y += bind.o
+ovpn-y += crypto.o
+ovpn-y += crypto_aead.o
ovpn-y += main.o
ovpn-y += io.o
ovpn-y += netlink.o
ovpn-y += netlink-gen.o
ovpn-y += peer.o
+ovpn-y += pktid.o
ovpn-y += socket.o
ovpn-y += udp.o
diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c
index
d4a1aeed12c99c71eaf5e8e9fc9c0fe61af6aaac..24d2788a277e674bde80b5aac9407c6528b108e5
100644
--- a/drivers/net/ovpn/bind.c
+++ b/drivers/net/ovpn/bind.c
@@ -48,11 +48,8 @@ struct ovpn_bind *ovpn_bind_from_sockaddr(const struct
sockaddr_storage *ss)
*/
void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *new)
{
- struct ovpn_bind *old;
+ lockdep_assert_held(&peer->lock);
- spin_lock_bh(&peer->lock);
- old = rcu_replace_pointer(peer->bind, new, true);
- spin_unlock_bh(&peer->lock);
-
- kfree_rcu(old, rcu);
+ kfree_rcu(rcu_replace_pointer(peer->bind, new,
+ lockdep_is_held(&peer->lock)), rcu);
}
diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
new file mode 100644
index
..ac15aaa9844083328020fcc5ea6866e08ecbc184
--- /dev/null
+++ b/drivers/net/ovpn/crypto.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/* OpenVPN data channel offload
+ *
+ * Copyright (C) 2020-2025 OpenVPN, Inc.
+ *
+ * Author:James Yonan
+ * Antonio Quartulli
+ */
+
+#include
+#include
+#include
+#include
+
+#include "ovpnpriv.h"
+#include "main.h"
+#include "pktid.h"
+#include "crypto_aead.h"
+#include "crypto.h"
+
+static void ovpn_ks_destroy_rcu(struct rcu_head *head)
+{
+ struct ovpn_crypto_key_slot *ks;
+
+ ks = container_of(head, struct ovpn_crypto_key_slot, rcu);
+ ovpn_aead_crypto_key_slot_destroy(ks);
+}
+
+void ovpn_crypto_key_slot_release(struct kref *kref)
+{
+ struct ovpn_crypto_key_slot *ks;
+
+ ks = container_of(kref, struct ovpn_crypto_key_slot, refcount);
+ call_rcu(&ks->rcu, ovpn_ks_destroy_rcu);
+}
+
+/* can only be invoked when all peer references have been dropped (i.e. RCU
+ * release routine)
+ */
+void ovpn_crypto_state_release(struct ovpn_crypto_state *cs)
+{
+ struct ovpn_crypto_key_slot *ks;
+
+ ks = rcu_access_pointer(cs->slots[0]);
+ if (ks) {
+ RCU_INIT_POINTER(cs->slots[0], NULL);
+ ovpn_crypto_key_slot_put(ks);
+ }
+
+ ks = rcu_access_pointer(cs->slots[1]);
+ if (ks) {
+ RCU_INIT_POINTER(cs->slots[1], NULL);
+ ovpn_crypto_key_slot_put(ks);
+ }
+}
+
+/* Reset the ovpn_crypto_state object in a way that is atomic
+ * to RCU readers.
+ */
+int ovpn_crypto_state_reset(struct ovpn_crypto_state *cs,
+ const struct ovpn_peer_key_reset *pkr)
+{
+ struct ovpn_crypto_key_slot *old = NULL, *new;
+ u8 idx;
+
+ if (pkr->slot != OVPN_KEY_SLOT_PRIMARY &&
+ pkr->slot != OVPN_KEY_SLOT_SECONDARY)
+ return -EINVAL;
+
+ new = ovpn_aead_crypto_key_slot_new(&pkr->key);
+ if (IS_ERR(new))
+ return P
