Re: [PATCH net-next v3 11/24] ovpn: implement packet processing

[Sabrina Dubroca <sd@queasysnail.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