diff mbox series

[Openvpn-devel,v3] Fix update_time() and openvpn_gettimeofday() coexistence

Message ID 20200922170841.13729-1-themiron@yandex-team.ru
State Accepted
Headers
Series [Openvpn-devel,v3] Fix update_time() and openvpn_gettimeofday() coexistence |

Commit Message

Vladislav Grishenko Sept. 22, 2020, 7:08 a.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 openvpn_gettimeofday() results and leads to time jumps in the
future within one second, can affect shaper and user timers.

Example:
100.900 openvpn_gettimeofday():
    now set to 100s, now_usec set to 900ns, 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,
there're several options to fix the things:

1. Allow update_time() to reset "now_usec" value backward to 0, since it's
   used directly only in time ajusting and always invalidate it in
   openvpn_gettimeofday() unless time has drifted backwards.
   Quick solution that only fixes openvpn_gettimeofday() and keeps current
   level of time performance and backward-protection handling way.

2. Switch update_time() to gettimeofday() not only for windows, but for all
   platforms: "now_usec" will be updated accordingly. As a disadvantage,
   gettimeofday() may have performance penalty on older or platforms w/o VDSO
   where expensive kernel syscall will be made. And it will still need time
   adjusting code, doubt it's feasible.

3. Switch update_time() and openvpn_gettimeofday() to clock_gettime() on
   Linux/BSD platforms with CLOCK_REALTIME_FAST/CLOCK_REALTIME_COARSE
   clock sources. According tests it'll be faster with VDSO than gettimeofday()
   or CLOCK_REALTIME/CLOCK_REALTIME_PRECISE, but still may require adjusting
   code to protect from time jumps on devices with no RTC (ex. routers) where
   NTP is the only way to get correct time after boot. Since not every *libc
   have clock_gettime() and corresponding CLOCK_* defines and/or running
   kernel may have no VDSO/corresponding CLOCK_* support - related autotools
   checks and fallback code can still be necessary.

4. Switch update_time() and openvpn_gettimeofday() to clock_gettime() on
   Linux/BSD platforms with CLOCK_MONOTONIC_FAST/CLOCK_MONOTONIC_COARSE
   clock sources. This may allow to get rid of time adjusting code at all
   with the acceptable performance on modern systems, but may still require
   to fallback to gettimeofday() with adj friends on older platforms (most
   likely to be Linux CPE/routers). From the effort point of view, splitting
   the whole OpenVPN code into realtime/monotonic is most significant and
   desired task among the above, several parts still needs to use realtime
   due API or storage or output reasons.

This patch implements the first stage only.

v2: move from gettimeofday() (1st way) back to time(), don't check previous
    value of "now_usec" in update_usec() instead
v3: recover "now_usec" checks against time jumps within one second, zero it
    in update_time() calls instead to pass the check.

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

Comments

Gert Doering Sept. 29, 2020, 12:18 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

I think this is a good first patch for the problem at hand, and it 
might actually fix one of our (old!) open trac tickets, claiming that 
the shaper is not (always) working correctly on Linux - Trac#91

For master, we should look more closely into always using "one of
the usec providing" time functions, and not mixing time() and
gettimeofday() - at least on platforms where this can be done 
without adverse effects, even if virtualization is involved
(making *some* time related functions horribly slow).

I have read the context (otime.h, otime.c), agree with your
conclusions, *and* have tested it client-side on Linux and
FreeBSD - nothing broke.  I have not tested the shaper.

Your patch has been applied to the master and release/2.5 branch.

commit e9e47f498674f6db8c3b88b32c877c5beb09a888 (master)
commit 51ee3574faade4dae5762140ea9f3a5eadc9d1e3 (release/2.5)
Author: Vladislav Grishenko
Date:   Tue Sep 22 22:08:41 2020 +0500

     Fix update_time() and openvpn_gettimeofday() coexistence

     Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200922170841.13729-1-themiron@yandex-team.ru>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21070.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/otime.h b/src/openvpn/otime.h
index a6f7ec25..78d20ba8 100644
--- a/src/openvpn/otime.h
+++ b/src/openvpn/otime.h
@@ -84,6 +84,7 @@  update_time(void)
     openvpn_gettimeofday(&tv, NULL);
 #else
     update_now(time(NULL));
+    now_usec = 0;
 #endif
 }