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

[Sabrina Dubroca <sd@queasysnail.net>]

2024-11-12, 14:20:45 +0100, Antonio Quartulli wrote:
> On 05/11/2024 19:10, Sabrina Dubroca wrote:
> > 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).
> 
> Is that because last_recv is 64 bit long (and might be more than one word on
> certain architectures)?
> 
> I don't remember having to do so for reading/writing 32 bit long integers.

AFAIK it's not just that. The compiler is free to do the read/write in
any way it wants when you don't specify _ONCE. On the read side, it
could read from memory a single time or multiple times (getting
possibly different values each time), or maybe split the load
(possibly reading chunks from different values being written in
parallel).

> I presume we need a WRITE_ONCE also upon initialization in
> ovpn_peer_keepalive_set() right?
> We still want to coordinate that with other reads/writes.

I think it makes sense, yes.

> > > +
> > >   	/* 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.
> 
> you're absolutely right. Will change that.

Thanks.

> > > +	/* 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
> > 
> 
> Yeah, there is nothing preventing this from happening...but is this truly a
> problem? The math should still work, no?

We'll fail "delta < timeout" (which we shouldn't), so we'll end up
either in the "expired = true" case, or not updating
keepalive_recv_exp. Both of these seem not ideal.

> 
> However:
> 
> > 
> > 
> > > +	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;
> > > +	}
> 
> I agree this is simpler to read and gets rid of some extra operations.
> 
> [note: I took inspiration from nat_keepalive_work_single() - it could be
> simplified as well I guess]

Ah, ok. I wanted to review this code when it was posted but didn't
have time :(

> > 
> > [...]
> > > +	/* 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
> 
> Yeah, will change both. Thanks!

Thanks.

-- 
Sabrina