From: Sabrina Dubroca <sd@queasysnail.net>
To: Antonio Quartulli <antonio@openvpn.net>
Cc: netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
Sergey Ryazanov <ryazanov.s.a@gmail.com>,
Paolo Abeni <pabeni@redhat.com>,
Eric Dumazet <edumazet@google.com>, Andrew Lunn <andrew@lunn.ch>,
Esben Haabendal <esben@geanix.com>
Subject: Re: [PATCH net-next v3 11/24] ovpn: implement packet processing
Date: Sun, 12 May 2024 10:46:18 +0200 [thread overview]
Message-ID: <ZkCB2sFnpIluo3wm@hog> (raw)
In-Reply-To: <20240506011637.27272-12-antonio@openvpn.net>
2024-05-06, 03:16:24 +0200, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c
> index c1f842c06e32..7240d1036fb7 100644
> --- a/drivers/net/ovpn/bind.c
> +++ b/drivers/net/ovpn/bind.c
> @@ -13,6 +13,7 @@
> #include "ovpnstruct.h"
> #include "io.h"
> #include "bind.h"
> +#include "packet.h"
> #include "peer.h"
You have a few hunks like that in this patch, adding an include to a
file that is otherwise not being modified. That's odd.
> diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
> new file mode 100644
> index 000000000000..98ef1ceb75e0
> --- /dev/null
> +++ b/drivers/net/ovpn/crypto.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* OpenVPN data channel offload
> + *
> + * Copyright (C) 2020-2024 OpenVPN, Inc.
> + *
> + * Author: James Yonan <james@openvpn.net>
> + * Antonio Quartulli <antonio@openvpn.net>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/net.h>
> +#include <linux/netdevice.h>
> +//#include <linux/skbuff.h>
That's also odd :)
[...]
> +/* 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)
> + __must_hold(cs->mutex)
> +{
> + struct ovpn_crypto_key_slot *old = NULL;
> + struct ovpn_crypto_key_slot *new;
> +
> + lockdep_assert_held(&cs->mutex);
> +
> + new = ovpn_aead_crypto_key_slot_new(&pkr->key);
This doesn't need the lock to be held, you could move the lock to a
smaller section (only around the pointer swap).
> + if (IS_ERR(new))
> + return PTR_ERR(new);
> +
> + switch (pkr->slot) {
> + case OVPN_KEY_SLOT_PRIMARY:
> + old = rcu_replace_pointer(cs->primary, new,
> + lockdep_is_held(&cs->mutex));
> + break;
> + case OVPN_KEY_SLOT_SECONDARY:
> + old = rcu_replace_pointer(cs->secondary, new,
> + lockdep_is_held(&cs->mutex));
> + break;
> + default:
> + goto free_key;
And validating pkr->slot before alloc could avoid a pointless
alloc/free (and simplify the code: once _new() has succeeded, no
failure can occur anymore).
> + }
> +
> + if (old)
> + ovpn_crypto_key_slot_put(old);
> +
> + return 0;
> +free_key:
> + ovpn_crypto_key_slot_put(new);
> + return -EINVAL;
> +}
> +
> +void ovpn_crypto_key_slot_delete(struct ovpn_crypto_state *cs,
> + enum ovpn_key_slot slot)
> +{
> + struct ovpn_crypto_key_slot *ks = NULL;
> +
> + mutex_lock(&cs->mutex);
> + switch (slot) {
> + case OVPN_KEY_SLOT_PRIMARY:
> + ks = rcu_replace_pointer(cs->primary, NULL,
> + lockdep_is_held(&cs->mutex));
> + break;
> + case OVPN_KEY_SLOT_SECONDARY:
> + ks = rcu_replace_pointer(cs->secondary, NULL,
> + lockdep_is_held(&cs->mutex));
> + break;
> + default:
> + pr_warn("Invalid slot to release: %u\n", slot);
> + break;
> + }
> + mutex_unlock(&cs->mutex);
> +
> + if (!ks) {
> + pr_debug("Key slot already released: %u\n", slot);
This will also be printed in case of an invalid argument, which would
be mildly confusing.
> + return;
> + }
> + pr_debug("deleting key slot %u, key_id=%u\n", slot, ks->key_id);
> +
> + ovpn_crypto_key_slot_put(ks);
> +}
> +static struct ovpn_crypto_key_slot *
> +ovpn_aead_crypto_key_slot_init(enum ovpn_cipher_alg alg,
> + const unsigned char *encrypt_key,
> + unsigned int encrypt_keylen,
> + const unsigned char *decrypt_key,
> + unsigned int decrypt_keylen,
> + const unsigned char *encrypt_nonce_tail,
> + unsigned int encrypt_nonce_tail_len,
> + const unsigned char *decrypt_nonce_tail,
> + unsigned int decrypt_nonce_tail_len,
> + u16 key_id)
> +{
[...]
> +
> + if (sizeof(struct ovpn_nonce_tail) != encrypt_nonce_tail_len ||
> + sizeof(struct ovpn_nonce_tail) != decrypt_nonce_tail_len) {
> + ret = -EINVAL;
> + goto destroy_ks;
> + }
Those checks could be done earlier, before bothering with any
allocations.
> +
> + memcpy(ks->nonce_tail_xmit.u8, encrypt_nonce_tail,
> + sizeof(struct ovpn_nonce_tail));
> + memcpy(ks->nonce_tail_recv.u8, decrypt_nonce_tail,
> + sizeof(struct ovpn_nonce_tail));
> +
> + /* init packet ID generation/validation */
> + ovpn_pktid_xmit_init(&ks->pid_xmit);
> + ovpn_pktid_recv_init(&ks->pid_recv);
> +
> + return ks;
> +
> +destroy_ks:
> + ovpn_aead_crypto_key_slot_destroy(ks);
> + return ERR_PTR(ret);
> +}
> +
> +struct ovpn_crypto_key_slot *
> +ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc)
> +{
> + return ovpn_aead_crypto_key_slot_init(kc->cipher_alg,
> + kc->encrypt.cipher_key,
> + kc->encrypt.cipher_key_size,
> + kc->decrypt.cipher_key,
> + kc->decrypt.cipher_key_size,
> + kc->encrypt.nonce_tail,
> + kc->encrypt.nonce_tail_size,
> + kc->decrypt.nonce_tail,
> + kc->decrypt.nonce_tail_size,
> + kc->key_id);
> +}
Why the wrapper? You could just call ovpn_aead_crypto_key_slot_init
directly.
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index 9935a863bffe..66a4c551c191 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -110,6 +114,27 @@ int ovpn_napi_poll(struct napi_struct *napi, int budget)
> return work_done;
> }
>
> +/* Return IP protocol version from skb header.
> + * Return 0 if protocol is not IPv4/IPv6 or cannot be read.
> + */
> +static __be16 ovpn_ip_check_protocol(struct sk_buff *skb)
nit: if you put this function higher up in the patch that introduced
it, you wouldn't have to move it now
> +{
> + __be16 proto = 0;
> +
> + /* skb could be non-linear, make sure IP header is in non-fragmented
> + * part
> + */
> + if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
> + return 0;
> +
> + if (ip_hdr(skb)->version == 4)
> + proto = htons(ETH_P_IP);
> + else if (ip_hdr(skb)->version == 6)
> + proto = htons(ETH_P_IPV6);
> +
> + return proto;
> +}
> +
> /* Entry point for processing an incoming packet (in skb form)
> *
> * Enqueue the packet and schedule RX consumer.
> @@ -132,7 +157,81 @@ int ovpn_recv(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
>
> static int ovpn_decrypt_one(struct ovpn_peer *peer, struct sk_buff *skb)
> {
> - return true;
I missed that in the RX patch, true isn't an int :)
Were you intending this function to be bool like ovpn_encrypt_one?
Since you're not actually using the returned value in the caller, it
would be reasonable, but you'd have to convert all the <0 error values
to bool.
> + struct ovpn_peer *allowed_peer = NULL;
> + struct ovpn_crypto_key_slot *ks;
> + __be16 proto;
> + int ret = -1;
> + u8 key_id;
> +
> + /* get the key slot matching the key Id in the received packet */
> + key_id = ovpn_key_id_from_skb(skb);
> + ks = ovpn_crypto_key_id_to_slot(&peer->crypto, key_id);
> + if (unlikely(!ks)) {
> + net_info_ratelimited("%s: no available key for peer %u, key-id: %u\n",
> + peer->ovpn->dev->name, peer->id, key_id);
> + goto drop;
> + }
> +
> + /* decrypt */
> + ret = ovpn_aead_decrypt(ks, skb);
> +
> + ovpn_crypto_key_slot_put(ks);
> +
> + if (unlikely(ret < 0)) {
> + net_err_ratelimited("%s: error during decryption for peer %u, key-id %u: %d\n",
> + peer->ovpn->dev->name, peer->id, key_id,
> + ret);
> + goto drop;
> + }
> +
> + /* check if this is a valid datapacket that has to be delivered to the
> + * tun interface
s/tun/ovpn/ ?
> + */
> + skb_reset_network_header(skb);
> + proto = ovpn_ip_check_protocol(skb);
> + if (unlikely(!proto)) {
> + /* check if null packet */
> + if (unlikely(!pskb_may_pull(skb, 1))) {
> + netdev_dbg(peer->ovpn->dev,
> + "NULL packet received from peer %u\n",
> + peer->id);
> + ret = -EINVAL;
> + goto drop;
> + }
> +
> + netdev_dbg(peer->ovpn->dev,
> + "unsupported protocol received from peer %u\n",
> + peer->id);
> +
> + ret = -EPROTONOSUPPORT;
> + goto drop;
> + }
> + skb->protocol = proto;
> +
> + /* perform Reverse Path Filtering (RPF) */
> + allowed_peer = ovpn_peer_get_by_src(peer->ovpn, skb);
> + if (unlikely(allowed_peer != peer)) {
> + if (skb_protocol_to_family(skb) == AF_INET6)
> + net_warn_ratelimited("%s: RPF dropped packet from peer %u, src: %pI6c\n",
> + peer->ovpn->dev->name, peer->id,
> + &ipv6_hdr(skb)->saddr);
> + else
> + net_warn_ratelimited("%s: RPF dropped packet from peer %u, src: %pI4\n",
> + peer->ovpn->dev->name, peer->id,
> + &ip_hdr(skb)->saddr);
> + ret = -EPERM;
> + goto drop;
> + }
Have you considered holding rcu_read_lock around this whole RPF check?
It would avoid taking a reference on the peer just to release it 3
lines later. And the same could likely be done for some of the other
ovpn_peer_get_* lookups too.
> + ret = ptr_ring_produce_bh(&peer->netif_rx_ring, skb);
> +drop:
> + if (likely(allowed_peer))
> + ovpn_peer_put(allowed_peer);
> +
> + if (unlikely(ret < 0))
> + kfree_skb(skb);
> +
> + return ret;
Mixing the drop/success returns looks kind of strange. This would be a
bit simpler:
ovpn_peer_put(allowed_peer);
return ptr_ring_produce_bh(&peer->netif_rx_ring, skb);
drop:
if (allowed_peer)
ovpn_peer_put(allowed_peer);
kfree_skb(skb);
return ret;
> diff --git a/drivers/net/ovpn/packet.h b/drivers/net/ovpn/packet.h
> index 7ed146f5932a..e14c9bf464f7 100644
> --- a/drivers/net/ovpn/packet.h
> +++ b/drivers/net/ovpn/packet.h
> @@ -10,7 +10,7 @@
> #ifndef _NET_OVPN_PACKET_H_
> #define _NET_OVPN_PACKET_H_
>
> -/* When the OpenVPN protocol is ran in AEAD mode, use
> +/* When the OpenVPN protocol is run in AEAD mode, use
nit: that typo came in earlier
--
Sabrina
next prev parent reply other threads:[~2024-05-12 8:46 UTC|newest]
Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 1:16 [PATCH net-next v3 00/24] Introducing OpenVPN Data Channel Offload Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 01/24] netlink: add NLA_POLICY_MAX_LEN macro Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 02/24] net: introduce OpenVPN Data Channel Offload (ovpn) Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 03/24] ovpn: add basic netlink support Antonio Quartulli
2024-05-08 0:10 ` Jakub Kicinski
2024-05-08 7:42 ` Antonio Quartulli
2024-05-08 14:42 ` Sabrina Dubroca
2024-05-08 14:51 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 04/24] ovpn: add basic interface creation/destruction/management routines Antonio Quartulli
2024-05-08 0:18 ` Jakub Kicinski
2024-05-08 7:53 ` Antonio Quartulli
2024-05-08 14:52 ` Sabrina Dubroca
2024-05-09 1:06 ` Jakub Kicinski
2024-05-09 8:25 ` Antonio Quartulli
2024-05-09 10:09 ` Sabrina Dubroca
2024-05-09 10:35 ` Antonio Quartulli
2024-05-09 12:16 ` Sabrina Dubroca
2024-05-09 13:25 ` Antonio Quartulli
2024-05-09 13:52 ` Sabrina Dubroca
2024-05-06 1:16 ` [PATCH net-next v3 05/24] ovpn: implement interface creation/destruction via netlink Antonio Quartulli
2024-05-08 0:21 ` Jakub Kicinski
2024-05-08 9:49 ` Antonio Quartulli
2024-05-09 1:09 ` Jakub Kicinski
2024-05-09 8:30 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 06/24] ovpn: keep carrier always on Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 07/24] ovpn: introduce the ovpn_peer object Antonio Quartulli
2024-05-08 16:06 ` Sabrina Dubroca
2024-05-08 20:31 ` Antonio Quartulli
2024-05-09 13:04 ` Sabrina Dubroca
2024-05-09 13:24 ` Andrew Lunn
2024-05-10 18:57 ` Antonio Quartulli
2024-05-11 0:28 ` Jakub Kicinski
2024-05-09 13:44 ` Antonio Quartulli
2024-05-09 13:55 ` Andrew Lunn
2024-05-09 14:17 ` Sabrina Dubroca
2024-05-09 14:36 ` Antonio Quartulli
2024-05-09 14:53 ` Antonio Quartulli
2024-05-10 10:30 ` Sabrina Dubroca
2024-05-10 12:34 ` Antonio Quartulli
2024-05-10 14:11 ` Sabrina Dubroca
2024-05-13 10:09 ` Simon Horman
2024-05-13 10:53 ` Antonio Quartulli
2024-05-13 15:04 ` Simon Horman
2024-05-06 1:16 ` [PATCH net-next v3 08/24] ovpn: introduce the ovpn_socket object Antonio Quartulli
2024-05-08 17:10 ` Sabrina Dubroca
2024-05-08 20:38 ` Antonio Quartulli
2024-05-09 13:32 ` Sabrina Dubroca
2024-05-09 13:46 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 09/24] ovpn: implement basic TX path (UDP) Antonio Quartulli
2024-05-10 13:01 ` Sabrina Dubroca
2024-05-10 13:39 ` Antonio Quartulli
2024-05-12 21:35 ` Sabrina Dubroca
2024-05-13 7:37 ` Antonio Quartulli
2024-05-13 9:36 ` Sabrina Dubroca
2024-05-13 9:47 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 10/24] ovpn: implement basic RX " Antonio Quartulli
2024-05-10 13:45 ` Sabrina Dubroca
2024-05-10 14:41 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 11/24] ovpn: implement packet processing Antonio Quartulli
2024-05-12 8:46 ` Sabrina Dubroca [this message]
2024-05-13 7:14 ` Antonio Quartulli
2024-05-13 9:24 ` Sabrina Dubroca
2024-05-13 9:31 ` Antonio Quartulli
2024-05-22 14:08 ` Antonio Quartulli
2024-05-22 14:28 ` Andrew Lunn
2024-05-06 1:16 ` [PATCH net-next v3 12/24] ovpn: store tunnel and transport statistics Antonio Quartulli
2024-05-12 8:47 ` Sabrina Dubroca
2024-05-13 7:25 ` Antonio Quartulli
2024-05-13 9:19 ` Sabrina Dubroca
2024-05-13 9:33 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 13/24] ovpn: implement TCP transport Antonio Quartulli
2024-05-13 13:37 ` Antonio Quartulli
2024-05-13 15:34 ` Jakub Kicinski
2024-05-13 14:50 ` Sabrina Dubroca
2024-05-13 22:20 ` Antonio Quartulli
2024-05-14 8:58 ` Sabrina Dubroca
2024-05-14 22:11 ` Antonio Quartulli
2024-05-15 10:19 ` Sabrina Dubroca
2024-05-15 12:54 ` Antonio Quartulli
2024-05-15 14:55 ` Sabrina Dubroca
2024-05-15 19:44 ` Antonio Quartulli
2024-05-15 20:35 ` Sabrina Dubroca
2024-05-15 20:39 ` Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 14/24] ovpn: implement multi-peer support Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 15/24] ovpn: implement peer lookup logic Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 16/24] ovpn: implement keepalive mechanism Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 17/24] ovpn: add support for updating local UDP endpoint Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 18/24] ovpn: add support for peer floating Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 19/24] ovpn: implement peer add/dump/delete via netlink Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 20/24] ovpn: implement key add/del/swap " Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 21/24] ovpn: kill key and notify userspace in case of IV exhaustion Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 22/24] ovpn: notify userspace when a peer is deleted Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 23/24] ovpn: add basic ethtool support Antonio Quartulli
2024-05-06 1:16 ` [PATCH net-next v3 24/24] testing/selftest: add test tool and scripts for ovpn module Antonio Quartulli
2024-05-07 23:55 ` Jakub Kicinski
2024-05-08 9:51 ` Antonio Quartulli
2024-05-09 0:50 ` Jakub Kicinski
2024-05-09 8:40 ` Antonio Quartulli
2024-05-07 23:48 ` [PATCH net-next v3 00/24] Introducing OpenVPN Data Channel Offload Jakub Kicinski
2024-05-08 9:56 ` Antonio Quartulli
2024-05-09 0:53 ` Jakub Kicinski
2024-05-09 8:41 ` Antonio Quartulli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZkCB2sFnpIluo3wm@hog \
--to=sd@queasysnail.net \
--cc=andrew@lunn.ch \
--cc=antonio@openvpn.net \
--cc=edumazet@google.com \
--cc=esben@geanix.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=ryazanov.s.a@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).