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

[Sabrina Dubroca]

2024-11-14, 09:12:01 +0100, Antonio Quartulli wrote:
> 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:
> > > > > +	/* 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.

But timeout is "unsigned long", so the comparison will be done as
unsigned.

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

:)

-- 
Sabrina