On 13/11/2024 11:36, Sabrina Dubroca wrote:
2024-11-12, 14:20:45 +0100, Antonio Quartulli wrote:
On 05/11/2024 19:10, Sabrina Dubroca wrote:
2024-10-29, 11:47:28 +0100, Antonio Quartulli wrote:
@@ -105,6 +132,9 @@ void ovpn_decrypt_post(void *data, int ret)
goto drop;
}
+ /* keep track of last received authenticated packet for keepalive */
+ peer->last_recv = ktime_get_real_seconds();
It doesn't look like we're locking the peer here so that should be a
WRITE_ONCE() (and READ_ONCE(peer->last_recv) for all reads).
Is that because last_recv is 64 bit long (and might be more than one word on
certain architectures)?
I don't remember having to do so for reading/writing 32 bit long integers.
AFAIK it's not just that. The compiler is free to do the read/write in
any way it wants when you don't specify _ONCE. On the read side, it
could read from memory a single time or multiple times (getting
possibly different values each time), or maybe split the load
(possibly reading chunks from different values being written in
parallel).
Ok, thanks. Will switch to WRITE/READ_ONE then.
I presume we need a WRITE_ONCE also upon initialization in
ovpn_peer_keepalive_set() right?
We still want to coordinate that with other reads/writes.
I think it makes sense, yes.
ACK
[...]
+ /* check for peer timeout */
+ expired = false;
+ timeout = peer->keepalive_timeout;
+ delta = now - peer->last_recv;
I'm not sure that's always > 0 if we finish decrypting a packet just
as the workqueue starts:
ovpn_peer_keepalive_work
now = ...
ovpn_decrypt_post
peer->last_recv = ...
ovpn_peer_keepalive_work_single
delta: now < peer->last_recv
Yeah, there is nothing preventing this from happening...but is this truly a
problem? The math should still work, no?
We'll fail "delta < timeout" (which we shouldn't), so we'll end up
either in the "expired = true" case, or not updating
keepalive_recv_exp. Both of these seem not ideal.
delta is signed, so it'll end up being a negative value and "delta <
timeout" should not fail then. Unless I am missing something.
Anyway, this was just an exercise to understand what was going on.
I already changed the code as per your suggestion (the fact that we are
still discussing this chunk proves that it needed to be simplified :))
However:
+ if (delta < timeout) {
+ peer->keepalive_recv_exp = now + timeout - delta;
I'd shorten that to
peer->keepalive_recv_exp = peer->last_recv + timeout;
it's a bit more readable to my eyes and avoids risks of wrapping
values.
So I'd probably get rid of delta and go with:
last_recv = READ_ONCE(peer->last_recv)
if (now < last_recv + timeout) {
peer->keepalive_recv_exp = last_recv + timeout;
next_run1 = peer->keepalive_recv_exp;
} else if ...
+ next_run1 = peer->keepalive_recv_exp;
+ } else if (peer->keepalive_recv_exp > now) {
+ next_run1 = peer->keepalive_recv_exp;
+ } else {
+ expired = true;
+ }
I agree this is simpler to read and gets rid of some extra operations.
[note: I took inspiration from nat_keepalive_work_single() - it could be
simplified as well I guess]
Ah, ok. I wanted to review this code when it was posted but didn't
have time :(
It can still be fixed ;)
Thanks.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
