RE: [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array
Hi Rafael, > -Original Message- > From: Rafael J. Wysocki [mailto:r...@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: > > > > 81c9d080 b sleep_time_bin > > 81c9d100 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 <cosu...@gmail.com> > > Signed-off-by: Chen Yu <yu.c.c...@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
RE: [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array
Hi Rafael, > -Original Message- > From: Rafael J. Wysocki [mailto:r...@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: > > > > 81c9d080 b sleep_time_bin > > 81c9d100 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 > > Signed-off-by: Chen Yu > > --- > > 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
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: > > 81c9d080 b sleep_time_bin > 81c9d100 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> Signed-off-by: Chen Yu > --- > 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? Thanks, Rafael
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: > > 81c9d080 b sleep_time_bin > 81c9d100 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 > Signed-off-by: Chen Yu > --- > 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? Thanks, Rafael
[PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array
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: 81c9d080 b sleep_time_bin 81c9d100 B dma_spin_lock Thus set the dma_spin_lock.val to 1, which caused this problem. 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 KozickiSigned-off-by: Chen Yu --- 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)]++; } -- 2.7.4
[PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array
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: 81c9d080 b sleep_time_bin 81c9d100 B dma_spin_lock Thus set the dma_spin_lock.val to 1, which caused this problem. 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 Signed-off-by: Chen Yu --- 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)]++; } -- 2.7.4