From: Willy Tarreau <w@1wt.eu>
Date: Wed, 28 Apr 2021 15:31:22 +0000 (+0200)
Subject: BUG/MEDIUM: time: fix updating of global_now upon clock drift
X-Git-Tag: v2.4-dev18~18
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=fe16126accdc7cc7f289d759a281d967c7eafbee;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: time: fix updating of global_now upon clock drift

During commit 7e4a557f6 ("MINOR: time: change the global timeval and the
the global tick at once") the approach made sure that the new now_ms was
always higher than or equal to global_now_ms, but by forgetting the old
value. This can cause the first update to global_now_ms to fail if it's
already out of sync, going back into the loop, and the subsequent call
would then succeed due to commit 4d01f3dcd ("MINOR: time: avoid
overwriting the same values of global_now").

And if it goes out of sync, it will fail to update forever, as observed
by Ashley Penney in github issue #1194, causing incorrect freq counters
calculations everywhere. One possible trigger for this issue is one thread
spinning for a few milliseconds while the other ones continue to work.

The issue really is that old_now_ms ought not to be modified in the loop
as it's used for the CAS. But we don't need to structurally guarantee that
global_now_ms grows monotonically as it's computed from the new global_now
which is already verified for this via the __tv_islt() test. Thus, dropping
any corrections on global_now_ms in the loop is the correct way to proceed
as long as this one is always updated to follow global_now.

No backport is needed, this is only for 2.4-dev.
---

diff --git a/src/time.c b/src/time.c
index 4ebde3779f..d6ab185b41 100644
--- a/src/time.c
+++ b/src/time.c
@@ -236,7 +236,6 @@ void tv_update_date(int max_wait, int interrupted)
 	do {
 		tmp_now.tv_sec  = (unsigned int)(old_now >> 32);
 		tmp_now.tv_usec = old_now & 0xFFFFFFFFU;
-		old_now_ms = __tv_to_ms(&tmp_now);
 
 		if (__tv_islt(&now, &tmp_now))
 			now = tmp_now;
@@ -246,8 +245,6 @@ void tv_update_date(int max_wait, int interrupted)
 		 */
 		new_now = ((ullong)now.tv_sec << 32) + (uint)now.tv_usec;
 		now_ms = __tv_to_ms(&now);
-		if (tick_is_lt(now_ms, old_now_ms))
-			now_ms = old_now_ms;
 
 		/* let's try to update the global <now> (both in timeval
 		 * and ms forms) or loop again.