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). > + > /* point to encapsulated IP packet */ > __skb_pull(skb, payload_offset); > > @@ -121,6 +151,12 @@ void ovpn_decrypt_post(void *data, int ret) > goto drop; > } > > + if (ovpn_is_keepalive(skb)) { > + net_dbg_ratelimited("%s: ping received from peer %u\n", > + peer->ovpn->dev->name, peer->id); > + goto drop; To help with debugging connectivity issues, maybe keepalives shouldn't be counted as drops? (consume_skb instead of kfree_skb, and not incrementing rx_dropped) The packet was successfully received and did all it had to do. > + } > + > net_info_ratelimited("%s: unsupported protocol received from peer %u\n", > peer->ovpn->dev->name, peer->id); > goto drop; > @@ -221,6 +257,10 @@ void ovpn_encrypt_post(void *data, int ret) > /* no transport configured yet */ > goto err; > } > + > + /* keep track of last sent packet for keepalive */ > + peer->last_sent = ktime_get_real_seconds(); And another WRITE_ONCE() here (also paired with READ_ONCE() on the read side). > +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); > + return ovpn_peer_del_mp(peer, reason); > + case OVPN_MODE_P2P: and here lockdep_assert_held(&peer->ovpn->lock); (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) > + 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 > + 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; > + } [...] > + /* 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 -- Sabrina