On 12/11/2024 14:20, Antonio Quartulli wrote: [...]
+static int ovpn_peer_del_nolock(struct ovpn_peer *peer, + enum ovpn_del_peer_reason reason) +{ + switch (peer->ovpn->mode) { + case OVPN_MODE_MP:
I think it would be nice to add
lockdep_assert_held(&peer->ovpn->peers->lock);
Sabrina, in other places I have used the sparse notation __must_hold() instead. Is there any preference in regards to lockdep vs sparse?
I could switch them all to lockdep_assert_held if needed.
Regards,
+ return ovpn_peer_del_mp(peer, reason); + case OVPN_MODE_P2P:
and here
lockdep_assert_held(&peer->ovpn->lock);
Yeah, good idea. __must_hold() can't work here, so lockdep_assert_held is definitely the way to go.
(I had to check that ovpn_peer_del_nolock is indeed called with those locks held since they're taken by ovpn_peer_keepalive_work_{mp,p2p}, adding these assertions would make it clear that ovpn_peer_del_nolock is not an unsafe version of ovpn_peer_del)
Right, it makes sense.
+ return ovpn_peer_del_p2p(peer, reason); + default: + return -EOPNOTSUPP; + } +}
/** * ovpn_peers_free - free all peers in the instance * @ovpn: the instance whose peers should be released @@ -830,3 +871,150 @@ void ovpn_peers_free(struct ovpn_struct *ovpn) ovpn_peer_unhash(peer, OVPN_DEL_PEER_REASON_TEARDOWN); spin_unlock_bh(&ovpn->peers->lock); }
+static time64_t ovpn_peer_keepalive_work_single(struct ovpn_peer *peer, + time64_t now) +{ + time64_t next_run1, next_run2, delta; + unsigned long timeout, interval; + bool expired;
+ spin_lock_bh(&peer->lock); + /* we expect both timers to be configured at the same time, + * therefore bail out if either is not set + */ + if (!peer->keepalive_timeout || !peer->keepalive_interval) { + spin_unlock_bh(&peer->lock); + return 0; + }
+ /* 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?
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]
[...]
+ /* check for peer keepalive */ + expired = false; + interval = peer->keepalive_interval; + delta = now - peer->last_sent; + if (delta < interval) { + peer->keepalive_xmit_exp = now + interval - delta; + next_run2 = peer->keepalive_xmit_exp;
and same here
Yeah, will change both. Thanks!
Regards,