Hi Rafael,

-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
Sent: Tuesday, July 19, 2016 6:02 AM
To: Chen, Yu C
Cc: John Stultz; Thomas Gleixner; linux-kernel@vger.kernel.org; Linux PM list
Subject: Re: [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin
array

On Monday, July 18, 2016 09:42:19 AM Chen Yu wrote:

quoted

It is reported the hibernation fails at 2nd attempt, which hangs at
hibernate() -> syscore_resume() -> i8237A_resume()
-> claim_dma_lock(), because the lock has already been taken.
However there is actually no other process would like to grab this
lock on that problematic platform.

Further investigation shows that, the problem is caused by setting
/sys/power/pm_trace to 1 before the 1st hibernation, since once
pm_trace is enabled, the rtc becomes an unmeaningful value after
resumed, which might bring a significant long sleep time in
timekeeping_resume, thus in tk_debug_account_sleep_time, the delta of
timespec64 might exceed 32bit after commit 7d489d15ce4b ("timekeeping:
Convert timekeeping core to use timespec64s"), thus if the bit31
happened set to 1, the fls might return 32 and then we add 1 to
sleep_time_bin[32], which caused a memory overwritten. As System.map

shows:

quoted

ffffffff81c9d080 b sleep_time_bin
ffffffff81c9d100 B dma_spin_lock

Thus set the dma_spin_lock.val to 1, which caused this problem.

Nice catch!

quoted

This patch fixes this issue by extending sleep_time_bin to 64, and use
__fls to be fit for timespec64.

Fixes: 7d489d15ce4b ("timekeeping: Convert timekeeping core to use
timespec64s")
Reported-and-tested-by: Janek Kozicki <redacted>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 kernel/time/timekeeping_debug.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/time/timekeeping_debug.c

b/kernel/time/timekeeping_debug.c index f6bd652..12b07d5 100644

--- a/kernel/time/timekeeping_debug.c
+++ b/kernel/time/timekeeping_debug.c

@@ -23,14 +23,14 @@

 #include "timekeeping_internal.h"

-static unsigned int sleep_time_bin[32] = {0};
+static unsigned int sleep_time_bin[64] = {0};

 static int tk_debug_show_sleep_time(struct seq_file *s, void *data)

{
 	unsigned int bin;
 	seq_puts(s, "      time (secs)        count\n");
 	seq_puts(s, "------------------------------\n");
-	for (bin = 0; bin < 32; bin++) {
+	for (bin = 0; bin < 64; bin++) {
 		if (sleep_time_bin[bin] == 0)
 			continue;
 		seq_printf(s, "%10u - %-10u %4u\n", @@ -69,6 +69,7 @@
late_initcall(tk_debug_sleep_time_init);

 void tk_debug_account_sleep_time(struct timespec64 *t)  {
-	sleep_time_bin[fls(t->tv_sec)]++;
+	if (t->tv_sec > 0)
+		sleep_time_bin[__fls(t->tv_sec)]++;

But you could simply validate t->tv_sec here without extending sleeo_time_bin[]
and switching over to __fls(), couldn't you?

Yes, we can check the maximal value for tv_sec, and don't have to extend sleep_time_bin array.

thanks,
Yu