Message-ID: <9ab97386-b83d-484e-8e6d-9f67a325669d@openvpn.net>
Date: Fri, 6 Sep 2024 15:19:08 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v6 12/25] ovpn: implement packet processing

On 04/09/2024 17:01, Sabrina Dubroca wrote:
> 2024-09-04, 14:07:23 +0200, Antonio Quartulli wrote:
>> Hi,
>>
>> On 02/09/2024 16:42, Sabrina Dubroca wrote:
>>> 2024-08-27, 14:07:52 +0200, Antonio Quartulli wrote:
>>>> +/* this swap is not atomic, but there will be a very short time frame where the
>>>
>>> Since we're under a mutex, I think we might get put to sleep for a
>>> not-so-short time frame.
>>>
>>>> + * old_secondary key won't be available. This should not be a big deal as most
>>>
>>> I could be misreading the code, but isn't it old_primary that's
>>> unavailable during the swap? rcu_replace_pointer overwrites
>>> cs->primary, so before the final assign, both slots contain
>>> old_secondary?
>>
>> Right. The comment is not correct.
>>
>> cs->secondary (old_secondary, that is the newest key) is what is probably
>> being used by the other peer for sending traffic.
> 
> Right, thanks. I was getting confused about the key slots and which
> key was the newest.
> 
> If the peer has already started sending with the newest key, no
> problem. If we're swapping keys before our peer (or we're on a slow
> network and the peer's packets get delayed), we'll still be receiving
> packets encrypted with the old key.

Right, it's mostly impossible to make the swap happen at the same time 
on both ends.

> 
> 
>> Therefore old_secondary is what is likely to be needed.
>>
>> However, this is pure speculation and may not be accurate.
> 
> I can think of a few possibilities if this causes too many unwanted
> drops:
> 
>   - use a linked list of keys, set the primary instead of swapping, and
>     let delete remove the unused key(s) by ID instead of slot
> 
>   - decouple the TX and RX keys, which also means you don't really need
>     to swap keys (swap for the TX key becomes "set primary", swap on RX
>     can become a noop since you check both slots for the correct keyid)
>     -- and here too delete becomes based on key ID
> 
>   - if cs->mutex becomes a spinlock, take it in the datapath when
>     looking up keys. this will make sure we get a consistent view of
>     the keys state.
> 
>   - come up with a scheme to let the datapath retry the key lookup if
>     it didn't find the key it wanted (maybe something like a seqcount,
>     or maybe taking the lock and retrying if the lookup failed)
> 
> I don't know if options 1 and 2 are possible based on how openvpn (the
> protocol and the userspace application) models keys, but they seem a
> bit "cleaner" on the datapath side (no locking, no retry). But they
> require a different API.

After chewing over all these ideas I think we can summarize the 
requirements as follows:

* PRIMARY and SECONDARY in the API is just an abstraction for "KEY FOR 
TX" and "THE OTHER KEY";
* SWAP means "mark for TX the key that currently was not marked";
* we only need a pointer to the key for TX;
* key for RX is picked up by key ID;

Therefore, how about having an array large enough to store all key IDs 
(max ID is 7):
* array index is the key ID (fast lookup on RX);
* upon SET_KEY we store the provided keys and erase the rest;
* upon SET_KEY we store in a new field the index/ID of the PRIMARY (fast 
lookup on TX), namely primary_id;
* upon SWAP we just save in primary_id the "other" index/ID;
* at any given time we will have only two keys in the array.

It's pretty much like your option 1 and 2, but using an array indexed by 
key ID.

The concept of slot is a bit lost, but it is not important as long as we 
can keep the API and its semantics the same.


Opinions?


> 
> Do you have the same problem in the current userspace implementation?

userspace is single-threaded :-P
nothing happens while we are busy swapping.

> 
> 
>> The fact that we could sleep before having completed the swap sounds like
>> something we want to avoid.
>> Maybe I should convert this mutex to a spinlock. Its usage is fairly
>> contained anyway.
> 
> I think it would make sense. It's only being held for very short
> periods, just to set/swap a few pointers.

ACK

> 
> 
>> FTR: this restructuring is the result of having tested encryption/decryption
>> with pcrypt: sg, that is passed to the crypto code, was initially allocated
>> on the stack, which was obviously not working for async crypto.
>> The solution was to make it part of the skb CB area, so that it can be
>> carried around until crypto is done.
> 
> I see. I thought this patch looked less familiar than the others :)
> 
> An alternative to using the CB is what IPsec does: allocate a chunk of
> memory for all its temporary needs (crypto req, sg, iv, anything else
> it needs during async crypto) and carve the pointers/small chunks out
> of it. See esp_alloc_tmp in net/ipv4/esp4.c.  (I'm just mentioning
> that for reference/curiosity, not asking that you change ovpn)
> 
> 
>> This patch was basically re-written after realizing that the async crypto
>> path was not working as expected, therefore sorry if there were some "kinda
>> obvious" mistakes.
> 
> And I completely missed some of those issues in previous reviews.
> 
>> Thanks a lot for your review.
> 
> Cheers, and thanks for your patience.
> 

Cheers!

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists