* [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).