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

[Sabrina Dubroca <sd@queasysnail.net>]

2024-05-13, 09:14:39 +0200, Antonio Quartulli wrote:
> 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.

Yep, I understand.

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


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


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