2024-07-17, 17:30:17 +0200, Antonio Quartulli wrote:

Hi,

On 15/07/2024 16:44, Sabrina Dubroca wrote:

quoted

2024-06-27, 15:08:35 +0200, Antonio Quartulli wrote:

quoted

+/**
+ * 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?

"Reasonable number" which should be enough[tm] to hold the entire packet.

Ok, let's go with that for now, unless someone else wants you to
change it.

quoted

quoted

+	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)

This will be part of a more sophisticated error counting that I will
introduce later on.

Cool, thanks.

quoted

quoted

+/**
+ * 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).

Normally this timer is only set upon connection, or maybe upon some future
parameter exchange. In both cases we can assume the connection is alive, so
this case should not scare us.

But thanks for pointing it out

Ok, I was thinking about updates while the connection is fully
established. If it's only done during setup, it shouldn't be a
problem.

quoted

quoted

+/**
+ * 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.

May it even introduce some performance penalty?

That's what I was worried about, yes.

I asked Paolo, he suggested checking that we're actually doing any
change to the timer:

   if (new_timeout_time != old_timeout_time)
       mod_timer(...)

This would reduce the update frequency to one per jiffy, which should
be acceptable.

Maybe we should get rid of the timer object and introduce a periodic (1s)
worker which checks some last_recv timestamp on every known peer?
What do you think?

That should work, or the workqueue like Eyal is saying.

-- 
Sabrina