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

Antonio Quartulli
2024-05-13T09:30:16Z

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.