2024-05-13, 09:14:39 +0200, Antonio Quartulli wrote:

On 12/05/2024 10:46, Sabrina Dubroca wrote:

quoted

2024-05-06, 03:16:24 +0200, Antonio Quartulli wrote:

quoted

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.

Yep, I understand.

quoted

quoted

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

But that wrapper doesn't really do anything.

In case my previous comment wasn't clear: I would keep the single
argument at the callsite (whether it's called _new or _init), and kill
the 10-args variant (it's too verbose and _very_ easy to mess up).

quoted

quoted

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

Yes please. If you can make the returns consistent (on success, one
returns true and the other returns 0), it would be nice.

quoted

quoted

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

Scratch that, it's broken (we'd leak the skb if ptr_ring_produce_bh
fails). Let's keep your version.

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

I presume it is mostly a matter of taste.

Maybe. As a reader I find it confusing to land into the "drop" label
on success and conditionally free the skb.

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.

If it's 90%, yes. Here, it looked like very little common code.

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

That won't be needed if you don't take a reference. Anyway,
netif_rx_ring will be gone if you switch to gro_cells, so that code is
likely to change.

-- 
Sabrina