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,
--
Antonio Quartulli
OpenVPN Inc.
