diff mbox series

[Openvpn-devel] Fix update_time() and openvpn_gettimeofday()

Message ID 20200922004005.10268-1-themiron@yandex-team.ru
State Superseded
Headers
Series [Openvpn-devel] Fix update_time() and openvpn_gettimeofday() |

Commit Message

Vladislav Grishenko Sept. 21, 2020, 2:40 p.m. UTC 
  With TIME_BACKTRACK_PROTECTION defined, openvpn_gettimeofday() uses and
updates global variable "now_usec" along with "now" only if current time
is ahead of the previsouly stored, taking nanoseconds into account.
But, update_time() function updates only "now" leaving "now_usec" as
is with any previously value stored.
This breaks both update_time() and openvpn_gettimeofday() results and
leads to time jumps in the future within one second, can affect shaper
and user timers.

100.900 openvpn_gettimeofday():
    now set to 100s, now_usec set to 100ns, stored time is 100.900
101.300 update_time():
    now set to 101s, but now_usec is not updated and still 900ns, stored
    time jumps to the future 101.900
101.600 openvpn_gettimeofday():
    current time 101.600 is in the past relatively stored time 101.900,
    now & now_usec variables are not updated, returned time 101.900 is
    still and again incorrect
102.100 openvpn_gettimeofday():
    current time 102.100 is no longer in the past relatively stored time
    101.900, so now & now_usec get updated with wrong time delta from
    previous openvpn_gettimeofday() call or now/now_usec math

Since update_time() and openvpn_gettimeofday() calls are mixed in runtime,
to fix their coexistance update_time() must update "now_usec" as well,
calling just update_now() is not enough.

Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
---
 src/openvpn/otime.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Arne Schwabe Sept. 21, 2020, 10:41 p.m. UTC | #1 
> --- a/src/openvpn/otime.h
> +++ b/src/openvpn/otime.h
> @@ -78,13 +78,9 @@ openvpn_gettimeofday(struct timeval *tv, void *tz)
>  static inline void
>  update_time(void)
>  {
> -#ifdef _WIN32
> -    /* on _WIN32, gettimeofday is faster than time(NULL) */
> +    /* can't use time(NULL), now_usec needs to be updated */
>      struct timeval tv;
>      openvpn_gettimeofday(&tv, NULL);
> -#else
> -    update_now(time(NULL));
> -#endif
>  }
>  
>  #else /* !TIME_BACKTRACK_PROTECTION */
> 

I am hesitant about this change, it mentions that gettimeofday is faster
on Windows than time(NULL) and we prefer it there for this reason. But
that also implies it is the other way round on other platforms. The
update_time function is called in multiple critical places. Do we have
any idea about this? I remember vaguely that the default gettimeofday in
FreeBSD is more accurate than the Linux variant (involves a kernel call)
but you can request a version similar to the Linux one by some flag
(reading from a shared readonly memory page that is read only).

Arne
Vladislav Grishenko Sept. 21, 2020, 11:34 p.m. UTC | #2 
Hi Arne,

> But that also
> implies it is the other way round on other platforms. The update_time function is
> called in multiple critical places. Do we have any idea about this?

On Linux with glibc's implementation both time() and gettimeofday() results in the same clock_gettime() call over VDSO (https://man7.org/linux/man-pages/man7/vdso.7.html).
On FreeBSD (and so), same if VDSO is used both time() and gettimeofday() will use clock_gettime too with even additional "tv->tv_usec = 0" for time().
So, on most of architectures there will be no difference after replacing time() to gettimeofday() except. asm commands for moving 32/64bit tv_usec, it'll be not visible.
On some acrhs/systems where VDSO is not supported - same syscall will happen.

> I remember
> vaguely that the default gettimeofday in FreeBSD is more accurate than the
> Linux variant (involves a kernel call) but you can request a version similar to the
> Linux one by some flag (reading from a shared readonly memory page that is
> read only).

As for clock drifting, it maybe feasible to use clock_gettime() with CLOCK_MONOTONIC and get rid of homegrown time adj code at all -> returned time will always be monotonic by design.
At least on supported platforms (!_WIN32).

--
Best Regards, Vladislav Grishenko

> -----Original Message-----
> From: Arne Schwabe <arne@rfc2549.org>
> Sent: Tuesday, September 22, 2020 1:41 PM
> To: Vladislav Grishenko <themiron@yandex-team.ru>; openvpn-
> devel@lists.sourceforge.net
> Subject: Re: [Openvpn-devel] [PATCH] Fix update_time() and
> openvpn_gettimeofday()
> 
> 
> > --- a/src/openvpn/otime.h
> > +++ b/src/openvpn/otime.h
> > @@ -78,13 +78,9 @@ openvpn_gettimeofday(struct timeval *tv, void *tz)
> > static inline void
> >  update_time(void)
> >  {
> > -#ifdef _WIN32
> > -    /* on _WIN32, gettimeofday is faster than time(NULL) */
> > +    /* can't use time(NULL), now_usec needs to be updated */
> >      struct timeval tv;
> >      openvpn_gettimeofday(&tv, NULL);
> > -#else
> > -    update_now(time(NULL));
> > -#endif
> >  }
> >
> >  #else /* !TIME_BACKTRACK_PROTECTION */
> >
> 
> I am hesitant about this change, it mentions that gettimeofday is faster on
> Windows than time(NULL) and we prefer it there for this reason. But that also
> implies it is the other way round on other platforms. The update_time function is
> called in multiple critical places. Do we have any idea about this? I remember
> vaguely that the default gettimeofday in FreeBSD is more accurate than the
> Linux variant (involves a kernel call) but you can request a version similar to the
> Linux one by some flag (reading from a shared readonly memory page that is
> read only).
> 
> Arne
diff mbox series

Patch

diff --git a/src/openvpn/otime.h b/src/openvpn/otime.h
index a6f7ec25..fab4575c 100644
--- a/src/openvpn/otime.h
+++ b/src/openvpn/otime.h
@@ -78,13 +78,9 @@  openvpn_gettimeofday(struct timeval *tv, void *tz)
 static inline void
 update_time(void)
 {
-#ifdef _WIN32
-    /* on _WIN32, gettimeofday is faster than time(NULL) */
+    /* can't use time(NULL), now_usec needs to be updated */
     struct timeval tv;
     openvpn_gettimeofday(&tv, NULL);
-#else
-    update_now(time(NULL));
-#endif
 }
 
 #else /* !TIME_BACKTRACK_PROTECTION */