linux-kernel - [PATCH v4 2/4] time: Fix a bug in timekeeping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sun, 15 Feb 2015 21:09:43 +0800
From:	Xunlei Pang <xlpang@....com>
To:	linux-kernel@...r.kernel.org
Cc:	rtc-linux@...glegroups.com, Thomas Gleixner <tglx@...utronix.de>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	John Stultz <john.stultz@...aro.org>,
	Arnd Bergmann <arnd.bergmann@...aro.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Xunlei Pang <pang.xunlei@...aro.org>
Subject: [PATCH v4 2/4] time: Fix a bug in timekeeping_suspend() with no persistent clock

From: Xunlei Pang <pang.xunlei@...aro.org>

When there's no persistent clock, normally timekeeping_suspend_time
should always be zero, but this can break in timekeeping_suspend().

At T1, there was a system suspend, so old_delta was assigned T1.
After some time, one time adjustment happened, and xtime got the
value of T1-dt(0s<dt<2s). Then, there comes another system suspend
soon after this adjustment, obviously we will get a small negative
delta_delta, resulting in a negative timekeeping_suspend_time.

This is problematic, when doing timekeeping_resume() if there is
no nonstop clocksource for example, it will hit the else leg and
inject the improper sleeptime which is the wrong logic.

So, we can solve this problem by only doing delta related code when
the persistent clock is existent. Actually the code only makes sense
for persistent clock cases.

This patch also improve the name of timekeeping_suspend_time.

Signed-off-by: Xunlei Pang <pang.xunlei@...aro.org>
---
 kernel/time/timekeeping.c | 50 ++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d78a528..ec6ee9b 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1100,8 +1100,8 @@ void __init timekeeping_init(void)
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 }
 
-/* time in seconds when suspend began */
-static struct timespec64 timekeeping_suspend_time;
+/* time in seconds when suspend began for persistent clock */
+static struct timespec64 persistent_clock_suspendtime;
 
 /**
  * __timekeeping_inject_sleeptime - Internal function to add sleep interval
@@ -1231,8 +1231,9 @@ static void timekeeping_resume(void)
 
 		ts_delta = ns_to_timespec64(nsec);
 		suspendtime_found = true;
-	} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
-		ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
+	} else if (timespec64_compare(&ts_new,
+				    &persistent_clock_suspendtime) > 0) {
+		ts_delta = timespec64_sub(ts_new, persistent_clock_suspendtime);
 		suspendtime_found = true;
 	}
 
@@ -1264,14 +1265,15 @@ static int timekeeping_suspend(void)
 	struct timespec tmp;
 
 	read_persistent_clock(&tmp);
-	timekeeping_suspend_time = timespec_to_timespec64(tmp);
+	persistent_clock_suspendtime = timespec_to_timespec64(tmp);
 
 	/*
 	 * On some systems the persistent_clock can not be detected at
 	 * timekeeping_init by its return value, so if we see a valid
 	 * value returned, update the persistent_clock_exists flag.
 	 */
-	if (timekeeping_suspend_time.tv_sec || timekeeping_suspend_time.tv_nsec)
+	if (persistent_clock_suspendtime.tv_sec ||
+	    persistent_clock_suspendtime.tv_nsec)
 		persistent_clock_exist = true;
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
@@ -1279,24 +1281,28 @@ static int timekeeping_suspend(void)
 	timekeeping_forward_now(tk);
 	timekeeping_suspended = 1;
 
-	/*
-	 * To avoid drift caused by repeated suspend/resumes,
-	 * which each can add ~1 second drift error,
-	 * try to compensate so the difference in system time
-	 * and persistent_clock time stays close to constant.
-	 */
-	delta = timespec64_sub(tk_xtime(tk), timekeeping_suspend_time);
-	delta_delta = timespec64_sub(delta, old_delta);
-	if (abs(delta_delta.tv_sec)  >= 2) {
+	if (has_persistent_clock()) {
 		/*
-		 * if delta_delta is too large, assume time correction
-		 * has occured and set old_delta to the current delta.
+		 * To avoid drift caused by repeated suspend/resumes,
+		 * which each can add ~1 second drift error,
+		 * try to compensate so the difference in system time
+		 * and persistent_clock time stays close to constant.
 		 */
-		old_delta = delta;
-	} else {
-		/* Otherwise try to adjust old_system to compensate */
-		timekeeping_suspend_time =
-			timespec64_add(timekeeping_suspend_time, delta_delta);
+		delta = timespec64_sub(tk_xtime(tk),
+					persistent_clock_suspendtime);
+		delta_delta = timespec64_sub(delta, old_delta);
+		if (abs(delta_delta.tv_sec)  >= 2) {
+			/*
+			 * if delta_delta is too large, assume time correction
+			 * has occurred and set old_delta to the current delta.
+			 */
+			old_delta = delta;
+		} else {
+			/* Otherwise try to adjust old_system to compensate */
+			persistent_clock_suspendtime =
+				timespec64_add(persistent_clock_suspendtime,
+					delta_delta);
+		}
 	}
 
 	timekeeping_update(tk, TK_MIRROR);
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/