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

[Antonio Quartulli]

On 13/11/2024 11:36, Sabrina Dubroca wrote:
> 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).

Ok, thanks. Will switch to WRITE/READ_ONE then.

> > 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.

ACK

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

delta is signed, so it'll end up being a negative value and "delta < 
timeout" should not fail then. Unless I am missing something.

Anyway, this was just an exercise to understand what was going on.
I already changed the code as per your suggestion (the fact that we are 
still discussing this chunk proves that it needed to be simplified :))

> > 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 :(

It can still be fixed ;)


Thanks.
Regards,

--
Antonio Quartulli
OpenVPN Inc.