| Message ID | 20200922170841.13729-1-themiron@yandex-team.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] Fix update_time() and openvpn_gettimeofday() coexistence | expand |
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 --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 }
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(+)