Re: [PATCH net-next v5 17/25] ovpn: implement keepalive mechanism

[Sabrina Dubroca <sd@queasysnail.net>]

2024-06-27, 15:08:35 +0200, Antonio Quartulli wrote:
> +static const unsigned char ovpn_keepalive_message[] = {
> +	0x2a, 0x18, 0x7b, 0xf3, 0x64, 0x1e, 0xb4, 0xcb,
> +	0x07, 0xed, 0x2d, 0x0a, 0x98, 0x1f, 0xc7, 0x48
> +};
> +
> +/**
> + * ovpn_is_keepalive - check if skb contains a keepalive message
> + * @skb: packet to check
> + *
> + * Assumes that the first byte of skb->data is defined.
> + *
> + * Return: true if skb contains a keepalive or false otherwise
> + */
> +static bool ovpn_is_keepalive(struct sk_buff *skb)
> +{
> +	if (*skb->data != OVPN_KEEPALIVE_FIRST_BYTE)

You could use ovpn_keepalive_message[0], and then you wouldn't need
this extra constant.

> +		return false;
> +
> +	if (!pskb_may_pull(skb, sizeof(ovpn_keepalive_message)))
> +		return false;
> +
> +	return !memcmp(skb->data, ovpn_keepalive_message,
> +		       sizeof(ovpn_keepalive_message));

Is a packet that contains some extra bytes after the exact keepalive
considered a valid keepalive, or does it need to be the correct
length?

> +}
> +
>  /* Called after decrypt to write the IP packet to the device.
>   * This method is expected to manage/free the skb.
>   */
> @@ -91,6 +116,9 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret)
>  		goto drop;
>  	}
>  
> +	/* note event of authenticated packet received for keepalive */
> +	ovpn_peer_keepalive_recv_reset(peer);
> +
>  	/* point to encapsulated IP packet */
>  	__skb_pull(skb, ovpn_skb_cb(skb)->payload_offset);
>  
> @@ -107,6 +135,12 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret)
>  			goto drop;
>  		}
>  
> +		if (ovpn_is_keepalive(skb)) {
> +			netdev_dbg(peer->ovpn->dev,
> +				   "ping received from peer %u\n", peer->id);

That should probably be _ratelimited, but it seems we don't have
_ratelimited variants for the netdev_* helpers.



> +/**
> + * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer
> + * @peer: peer to send the message to
> + * @data: message content
> + * @len: message length
> + *
> + * Assumes that caller holds a reference to peer
> + */
> +static void ovpn_xmit_special(struct ovpn_peer *peer, const void *data,
> +			      const unsigned int len)
> +{
> +	struct ovpn_struct *ovpn;
> +	struct sk_buff *skb;
> +
> +	ovpn = peer->ovpn;
> +	if (unlikely(!ovpn))
> +		return;
> +
> +	skb = alloc_skb(256 + len, GFP_ATOMIC);

Where is that 256 coming from?

> +	if (unlikely(!skb))
> +		return;

Failure to send a keepalive should probably have a counter, to help
users troubleshoot why their connection dropped.
(can be done later unless someone insists)


> +	skb_reserve(skb, 128);

And that 128?

> +	skb->priority = TC_PRIO_BESTEFFORT;
> +	memcpy(__skb_put(skb, len), data, len);

nit: that's __skb_put_data

> +	/* increase reference counter when passing peer to sending queue */
> +	if (!ovpn_peer_hold(peer)) {
> +		netdev_dbg(ovpn->dev, "%s: cannot hold peer reference for sending special packet\n",
> +			   __func__);
> +		kfree_skb(skb);
> +		return;
> +	}
> +
> +	ovpn_send(ovpn, skb, peer);
> +}
> +
> +/**
> + * ovpn_keepalive_xmit - send keepalive message to peer
> + * @peer: the peer to send the message to
> + */
> +void ovpn_keepalive_xmit(struct ovpn_peer *peer)
> +{
> +	ovpn_xmit_special(peer, ovpn_keepalive_message,
> +			  sizeof(ovpn_keepalive_message));
> +}

I don't see other users of ovpn_xmit_special in this series, if you
don't have more planned in the future you could drop the extra function.


> +/**
> + * ovpn_peer_expire - timer task for incoming keepialive timeout

typo: s/keepialive/keepalive/



> +/**
> + * ovpn_peer_keepalive_set - configure keepalive values for peer
> + * @peer: the peer to configure
> + * @interval: outgoing keepalive interval
> + * @timeout: incoming keepalive timeout
> + */
> +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout)
> +{
> +	u32 delta;
> +
> +	netdev_dbg(peer->ovpn->dev,
> +		   "%s: scheduling keepalive for peer %u: interval=%u timeout=%u\n",
> +		   __func__, peer->id, interval, timeout);
> +
> +	peer->keepalive_interval = interval;
> +	if (interval > 0) {
> +		delta = msecs_to_jiffies(interval * MSEC_PER_SEC);
> +		mod_timer(&peer->keepalive_xmit, jiffies + delta);

Maybe something to consider in the future: this could be resetting a
timer that was just about to go off to a somewhat distant time in the
future. Not sure the peer will be happy about that (and not consider
it a timeout).

> +	} else {
> +		timer_delete(&peer->keepalive_xmit);
> +	}
> +
> +	peer->keepalive_timeout = timeout;
> +	if (timeout) {

pedantic nit: inconsistent style with the "interval > 0" test just
above

> +		delta = msecs_to_jiffies(timeout * MSEC_PER_SEC);
> +		mod_timer(&peer->keepalive_recv, jiffies + delta);
> +	} else {
> +		timer_delete(&peer->keepalive_recv);
> +	}
> +}
> +

[...]
> +/**
> + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout
> + * @peer: peer for which the timeout should be reset
> + *
> + * To be invoked upon reception of an authenticated packet from peer in order
> + * to report valid activity and thus reset the keepalive timeout
> + */
> +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer)
> +{
> +	u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC);
> +
> +	if (unlikely(!delta))
> +		return;
> +
> +	mod_timer(&peer->keepalive_recv, jiffies + delta);

This (and ovpn_peer_keepalive_xmit_reset) is going to be called for
each packet. I wonder how well the timer subsystem deals with one
timer getting updated possibly thousands of time per second.

-- 
Sabrina