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

On 13/05/2024 11:24, Sabrina Dubroca wrote:

quoted

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

Oh ok, then I misunderstood your earlier comment.

Now it's clear and I totally agree. Originally there was a crypto 
abstraction in ovpn, to allow more crypto families later on.

But I deemed it being too complex and overkill.
This wrapper is a useless leftover of that approach.

Will get rid of this 10-args variant.

quoted

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.

I am normally all for int, as I don't like failing with no exact code.
Will most likely go with that.

quoted

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.

Right.

quoted

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.

quoted

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.

quoted

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.

Yap, working on gro_cells right now!

Thanks

-- 
Antonio Quartulli
OpenVPN Inc.