* [PATCH] locktorture: Fix NULL pointer when torture_type is invalid @ 2016-01-13 2:06 Kefeng Wang 2016-01-13 16:27 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Kefeng Wang @ 2016-01-13 2:06 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Paul E. McKenney, linux-kernel; +Cc: guohanjun Insmod locktorture with torture_type=mutex will lead to crash, Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = ffffffc0f6c10000 [00000008] *pgd=000000013b221003, *pud=000000013b221003, *pmd=0000000000000000 Internal error: Oops: 94000006 [#1] PREEMPT SMP Modules linked in: locktorture(+) torture CPU: 2 PID: 1462 Comm: insmod Not tainted 4.4.0+ #19 Hardware name: linux,dummy-virt (DT) task: ffffffc0fb2b3700 ti: ffffffc0fa938000 task.ti: ffffffc0fa938000 PC is at __torture_print_stats+0x18/0x180 [locktorture] LR is at lock_torture_stats_print+0x68/0x110 [locktorture] pc : [<ffffffbffc017028>] lr : [<ffffffbffc017500>] pstate: 60000145 sp : ffffffc0fa93bb20 [snip...] Call trace: [<ffffffbffc017028>] __torture_print_stats+0x18/0x180 [locktorture] [<ffffffbffc017500>] lock_torture_stats_print+0x68/0x110 [locktorture] [<ffffffbffc0180fc>] lock_torture_cleanup+0xc4/0x278 [locktorture] [<ffffffbffc01d144>] lock_torture_init+0x144/0x5b0 [locktorture] [<ffffffc000082940>] do_one_initcall+0x94/0x1a0 [<ffffffc000141888>] do_init_module+0x60/0x1c8 [<ffffffc00011c628>] load_module+0x1880/0x1c9c [<ffffffc00011cc00>] SyS_finit_module+0x7c/0x88 [<ffffffc000085cb0>] el0_svc_naked+0x24/0x28 Fix it by check stats_task in lock_torture_cleanup() to guard not into a wrong path. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- kernel/locking/locktorture.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c index 8ef1919..1afa8ad 100644 --- a/kernel/locking/locktorture.c +++ b/kernel/locking/locktorture.c @@ -764,8 +764,10 @@ static void lock_torture_cleanup(void) reader_tasks = NULL; } - torture_stop_kthread(lock_torture_stats, stats_task); - lock_torture_stats_print(); /* -After- the stats thread is stopped! */ + if (stats_task) { + torture_stop_kthread(lock_torture_stats, stats_task); + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ + } if (atomic_read(&cxt.n_lock_torture_errors)) lock_torture_print_module_parms(cxt.cur_ops, -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] locktorture: Fix NULL pointer when torture_type is invalid 2016-01-13 2:06 [PATCH] locktorture: Fix NULL pointer when torture_type is invalid Kefeng Wang @ 2016-01-13 16:27 ` Paul E. McKenney 2016-01-14 1:38 ` Kefeng Wang 0 siblings, 1 reply; 4+ messages in thread From: Paul E. McKenney @ 2016-01-13 16:27 UTC (permalink / raw) To: Kefeng Wang; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel, guohanjun On Wed, Jan 13, 2016 at 10:06:22AM +0800, Kefeng Wang wrote: > Insmod locktorture with torture_type=mutex will lead to crash, > > Unable to handle kernel NULL pointer dereference at virtual address 00000008 > pgd = ffffffc0f6c10000 > [00000008] *pgd=000000013b221003, *pud=000000013b221003, *pmd=0000000000000000 > Internal error: Oops: 94000006 [#1] PREEMPT SMP > Modules linked in: locktorture(+) torture > CPU: 2 PID: 1462 Comm: insmod Not tainted 4.4.0+ #19 > Hardware name: linux,dummy-virt (DT) > task: ffffffc0fb2b3700 ti: ffffffc0fa938000 task.ti: ffffffc0fa938000 > PC is at __torture_print_stats+0x18/0x180 [locktorture] > LR is at lock_torture_stats_print+0x68/0x110 [locktorture] > pc : [<ffffffbffc017028>] lr : [<ffffffbffc017500>] pstate: 60000145 > sp : ffffffc0fa93bb20 > [snip...] > Call trace: > [<ffffffbffc017028>] __torture_print_stats+0x18/0x180 [locktorture] > [<ffffffbffc017500>] lock_torture_stats_print+0x68/0x110 [locktorture] > [<ffffffbffc0180fc>] lock_torture_cleanup+0xc4/0x278 [locktorture] > [<ffffffbffc01d144>] lock_torture_init+0x144/0x5b0 [locktorture] > [<ffffffc000082940>] do_one_initcall+0x94/0x1a0 > [<ffffffc000141888>] do_init_module+0x60/0x1c8 > [<ffffffc00011c628>] load_module+0x1880/0x1c9c > [<ffffffc00011cc00>] SyS_finit_module+0x7c/0x88 > [<ffffffc000085cb0>] el0_svc_naked+0x24/0x28 > > Fix it by check stats_task in lock_torture_cleanup() to guard not into > a wrong path. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> Good catch! One question below... > --- > kernel/locking/locktorture.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c > index 8ef1919..1afa8ad 100644 > --- a/kernel/locking/locktorture.c > +++ b/kernel/locking/locktorture.c > @@ -764,8 +764,10 @@ static void lock_torture_cleanup(void) > reader_tasks = NULL; > } > > - torture_stop_kthread(lock_torture_stats, stats_task); > - lock_torture_stats_print(); /* -After- the stats thread is stopped! */ > + if (stats_task) { > + torture_stop_kthread(lock_torture_stats, stats_task); > + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ > + } Does it work to have the following? + if (stats_task) + torture_stop_kthread(lock_torture_stats, stats_task); + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ We need to print the statistics at the end of the run, even if we chose not to print them periodically throughout the run. Thanx, Paul > if (atomic_read(&cxt.n_lock_torture_errors)) > lock_torture_print_module_parms(cxt.cur_ops, > -- > 1.7.12.4 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] locktorture: Fix NULL pointer when torture_type is invalid 2016-01-13 16:27 ` Paul E. McKenney @ 2016-01-14 1:38 ` Kefeng Wang 2016-01-20 19:34 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Kefeng Wang @ 2016-01-14 1:38 UTC (permalink / raw) To: paulmck; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel, guohanjun Hi paul, On 2016/1/14 0:27, Paul E. McKenney wrote: > On Wed, Jan 13, 2016 at 10:06:22AM +0800, Kefeng Wang wrote: >> Insmod locktorture with torture_type=mutex will lead to crash, >> >> Unable to handle kernel NULL pointer dereference at virtual address 00000008 >> pgd = ffffffc0f6c10000 >> [00000008] *pgd=000000013b221003, *pud=000000013b221003, *pmd=0000000000000000 >> Internal error: Oops: 94000006 [#1] PREEMPT SMP >> Modules linked in: locktorture(+) torture >> CPU: 2 PID: 1462 Comm: insmod Not tainted 4.4.0+ #19 >> Hardware name: linux,dummy-virt (DT) >> task: ffffffc0fb2b3700 ti: ffffffc0fa938000 task.ti: ffffffc0fa938000 >> PC is at __torture_print_stats+0x18/0x180 [locktorture] >> LR is at lock_torture_stats_print+0x68/0x110 [locktorture] >> pc : [<ffffffbffc017028>] lr : [<ffffffbffc017500>] pstate: 60000145 >> sp : ffffffc0fa93bb20 >> [snip...] >> Call trace: >> [<ffffffbffc017028>] __torture_print_stats+0x18/0x180 [locktorture] >> [<ffffffbffc017500>] lock_torture_stats_print+0x68/0x110 [locktorture] >> [<ffffffbffc0180fc>] lock_torture_cleanup+0xc4/0x278 [locktorture] >> [<ffffffbffc01d144>] lock_torture_init+0x144/0x5b0 [locktorture] >> [<ffffffc000082940>] do_one_initcall+0x94/0x1a0 >> [<ffffffc000141888>] do_init_module+0x60/0x1c8 >> [<ffffffc00011c628>] load_module+0x1880/0x1c9c >> [<ffffffc00011cc00>] SyS_finit_module+0x7c/0x88 >> [<ffffffc000085cb0>] el0_svc_naked+0x24/0x28 >> >> Fix it by check stats_task in lock_torture_cleanup() to guard not into >> a wrong path. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > Good catch! One question below... > >> --- >> kernel/locking/locktorture.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c >> index 8ef1919..1afa8ad 100644 >> --- a/kernel/locking/locktorture.c >> +++ b/kernel/locking/locktorture.c >> @@ -764,8 +764,10 @@ static void lock_torture_cleanup(void) >> reader_tasks = NULL; >> } >> >> - torture_stop_kthread(lock_torture_stats, stats_task); >> - lock_torture_stats_print(); /* -After- the stats thread is stopped! */ >> + if (stats_task) { >> + torture_stop_kthread(lock_torture_stats, stats_task); >> + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ >> + } > > Does it work to have the following? > > + if (stats_task) > + torture_stop_kthread(lock_torture_stats, stats_task); > + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ > > We need to print the statistics at the end of the run, even if we chose > not to print them periodically throughout the run. > No, the pc is in __torture_print_stats(char *page, struct lock_stress_stats *statp, bool write), that is 'long min = statp[0].n_lock_acquired;', if the torture_type is invalid, the statp is NULL, because the cxt.lwsa and cxt.lwsa/cxt.lrsa are not initialized in lock_torture_init(). Thanks, Kefeng > Thanx, Paul > >> if (atomic_read(&cxt.n_lock_torture_errors)) >> lock_torture_print_module_parms(cxt.cur_ops, >> -- >> 1.7.12.4 >> > > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] locktorture: Fix NULL pointer when torture_type is invalid 2016-01-14 1:38 ` Kefeng Wang @ 2016-01-20 19:34 ` Paul E. McKenney 0 siblings, 0 replies; 4+ messages in thread From: Paul E. McKenney @ 2016-01-20 19:34 UTC (permalink / raw) To: Kefeng Wang; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel, guohanjun On Thu, Jan 14, 2016 at 09:38:04AM +0800, Kefeng Wang wrote: > Hi paul, > > On 2016/1/14 0:27, Paul E. McKenney wrote: > > On Wed, Jan 13, 2016 at 10:06:22AM +0800, Kefeng Wang wrote: > >> Insmod locktorture with torture_type=mutex will lead to crash, > >> > >> Unable to handle kernel NULL pointer dereference at virtual address 00000008 > >> pgd = ffffffc0f6c10000 > >> [00000008] *pgd=000000013b221003, *pud=000000013b221003, *pmd=0000000000000000 > >> Internal error: Oops: 94000006 [#1] PREEMPT SMP > >> Modules linked in: locktorture(+) torture > >> CPU: 2 PID: 1462 Comm: insmod Not tainted 4.4.0+ #19 > >> Hardware name: linux,dummy-virt (DT) > >> task: ffffffc0fb2b3700 ti: ffffffc0fa938000 task.ti: ffffffc0fa938000 > >> PC is at __torture_print_stats+0x18/0x180 [locktorture] > >> LR is at lock_torture_stats_print+0x68/0x110 [locktorture] > >> pc : [<ffffffbffc017028>] lr : [<ffffffbffc017500>] pstate: 60000145 > >> sp : ffffffc0fa93bb20 > >> [snip...] > >> Call trace: > >> [<ffffffbffc017028>] __torture_print_stats+0x18/0x180 [locktorture] > >> [<ffffffbffc017500>] lock_torture_stats_print+0x68/0x110 [locktorture] > >> [<ffffffbffc0180fc>] lock_torture_cleanup+0xc4/0x278 [locktorture] > >> [<ffffffbffc01d144>] lock_torture_init+0x144/0x5b0 [locktorture] > >> [<ffffffc000082940>] do_one_initcall+0x94/0x1a0 > >> [<ffffffc000141888>] do_init_module+0x60/0x1c8 > >> [<ffffffc00011c628>] load_module+0x1880/0x1c9c > >> [<ffffffc00011cc00>] SyS_finit_module+0x7c/0x88 > >> [<ffffffc000085cb0>] el0_svc_naked+0x24/0x28 > >> > >> Fix it by check stats_task in lock_torture_cleanup() to guard not into > >> a wrong path. > >> > >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > > > Good catch! One question below... > > > >> --- > >> kernel/locking/locktorture.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c > >> index 8ef1919..1afa8ad 100644 > >> --- a/kernel/locking/locktorture.c > >> +++ b/kernel/locking/locktorture.c > >> @@ -764,8 +764,10 @@ static void lock_torture_cleanup(void) > >> reader_tasks = NULL; > >> } > >> > >> - torture_stop_kthread(lock_torture_stats, stats_task); > >> - lock_torture_stats_print(); /* -After- the stats thread is stopped! */ > >> + if (stats_task) { > >> + torture_stop_kthread(lock_torture_stats, stats_task); > >> + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ > >> + } > > > > Does it work to have the following? > > > > + if (stats_task) > > + torture_stop_kthread(lock_torture_stats, stats_task); > > + lock_torture_stats_print(); /* -After- the stats thread is stopped! */ > > > > We need to print the statistics at the end of the run, even if we chose > > not to print them periodically throughout the run. > > > > No, the pc is in __torture_print_stats(char *page, struct lock_stress_stats *statp, bool write), > that is 'long min = statp[0].n_lock_acquired;', if the torture_type is invalid, the statp is NULL, > because the cxt.lwsa and cxt.lwsa/cxt.lrsa are not initialized in lock_torture_init(). Good point, thank you for checking! The reason that we don't have this problem in rcutorture is the lack of dynamically allocated statistics. It is really nice to get the statistics printout at the end, so it would be good for locktorture to keep it as well. Having that helps prevent false-positive test passes due to bugs that skip lock acquisitions and the like. So would it make sense to unconditionally allocate and free the memory, regardless of whether or not there was a stats kthread? Thanx, Paul ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-20 19:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-13 2:06 [PATCH] locktorture: Fix NULL pointer when torture_type is invalid Kefeng Wang 2016-01-13 16:27 ` Paul E. McKenney 2016-01-14 1:38 ` Kefeng Wang 2016-01-20 19:34 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).