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

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.

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.

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

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.

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.

-- 
Sabrina