Re: [PATCH net-next v11 15/23] ovpn: implement keepalive mechanism

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