Date: Mon, 13 May 2024 11:31:34 +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 13/05/2024 11:24, Sabrina Dubroca wrote:
>>>> +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.

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

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

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

Yap, working on gro_cells right now!

Thanks

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists