| Message ID | 20200922004005.10268-1-themiron@yandex-team.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Fix update_time() and openvpn_gettimeofday() | expand |
> --- 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
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 --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 */
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(-)