* RE: [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array
2016-07-18 22:01 ` [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array Rafael J. Wysocki
@ 2016-07-18 23:34 ` Chen, Yu C
0 siblings, 0 replies; 2+ messages in thread
From: Chen, Yu C @ 2016-07-18 23:34 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: John Stultz, Thomas Gleixner, linux-kernel@vger.kernel.org,
Linux PM list
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:
> > 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 <cosurgi@gmail.com>
> > 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
^ permalink raw reply [flat|nested] 2+ messages in thread