Message-ID: <d2733aaa-58fa-47da-a469-a848a6100759@openvpn.net>
Date: Mon, 13 May 2024 09:14:39 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
 Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 11/24] ovpn: implement packet processing

On 12/05/2024 10:46, Sabrina Dubroca wrote:
> 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.

Argh. The whole ovpn was originall a single patch, which I the went and 
divided in smaller changes for easier review.

As you may imagine this process is prone to mistakes like this, 
expecially when the number of patches is quite high...

I will go through all the patches and clean them up from issues like 
this and like the one below..

Sorry about that.

> 
>> 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@...nvpn.net>
>> + *		Antonio Quartulli <antonio@...nvpn.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).

I think you're right. I also like the idea of shrinking the lock active 
area.. Will fix this!


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

right! will fix

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

although we will have the pr_warn printed in as well that case.
But I agree this is not nice. will fix

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

ACK

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

Mostly for ahestetic reasons, being the call very large.
On top of that this is a little leftover from a previous version where 
this call happened more than once as part of an internal abstraction, 
hence the decision to create a wrapper.

But I think it's ok to remove it now.

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

ACK

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

Mhh let me think what's best and I wil make this uniform.

> 
>> +	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/ ?

yap. we used to call "tun" any interface used by openvpn...just legacy 
that fills our brains :-) will fix

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

thinking about this..you're right, because the peer object never leavs 
this context and therefore it is not stricly needed to hold the 
reference and do the full dance..

Sometimes I fear that I may envelope too many instructions within the 
rcu_read_lock and thus I go with the smallest area needed (a bit like a 
classic lock). But I agree that for these lookups this is not truly the 
case.

Will review the other lookups and change them accordingly.

Thanks!

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

Honestly I have seen this pattern fairly often (and implemented it this 
way fairly often).

I presume it is mostly a matter of taste.

The idea is: when exiting a function 90% of the code is shared between 
success and failure, therefore let's just write it once and simply add a 
few branches based on ret.

This way we have less code and if we need to chang somethig in the exit 
path, we can change it once only.

A few examples:
* 
https://elixir.bootlin.com/linux/v6.9-rc7/source/net/batman-adv/translation-table.c#L813
* 
https://elixir.bootlin.com/linux/v6.9-rc7/source/net/batman-adv/routing.c#L269
* https://elixir.bootlin.com/linux/v6.9-rc7/source/net/mac80211/scan.c#L1344


ovpn code can be further simplified by setting skb to NULL in case of 
success (this way we avoid checking ret) and let ovpn_peer_put handle 
the case of peer == NULL (we avoid the NULL check before calling it).

What do you think?


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

ops


Thanks!

> 

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists