On 04/09/2024 17:01, Sabrina Dubroca wrote:

2024-09-04, 14:07:23 +0200, Antonio Quartulli wrote:

quoted

Hi,

On 02/09/2024 16:42, Sabrina Dubroca wrote:

quoted

2024-08-27, 14:07:52 +0200, Antonio Quartulli wrote:

quoted

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

quoted

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

quoted

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.

quoted

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

quoted

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)

quoted

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.

quoted

Thanks a lot for your review.

Cheers, and thanks for your patience.

Cheers!

-- 
Antonio Quartulli
OpenVPN Inc.