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

[Sabrina Dubroca <sd@queasysnail.net>]

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