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

From: Sabrina Dubroca
Date: Thu Nov 14 2024 - 04:05:12 EST


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