Hi Rafael, > -----Original Message----- > From: Rafael J. Wysocki [mailto:[email protected]] > Sent: Tuesday, July 19, 2016 6:02 AM > To: Chen, Yu C > Cc: John Stultz; Thomas Gleixner; [email protected]; 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: > > 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: > > > > 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! > > > 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 <[email protected]> > > Signed-off-by: Chen Yu <[email protected]> > > --- > > 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
