Messages in this thread Patch in this message | | | Date | Sun, 31 Jan 2016 14:17:36 -0800 | From | Davidlohr Bueso <> | Subject | Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid |
| |
On Sat, 30 Jan 2016, Paul E. McKenney wrote:
>> On 2016/1/28 12:25, Kefeng Wang wrote: >> > Insmod locktorture with torture_type=mutex will lead to crash,
You actually want mutex_lock here, we always use the _lock suffix, mainly because it all started out with spin_lock. And you just showed how fragile this is -- I'd say most of use use this module in a scripted setup, which is why it was not seen before.
>> > >> > Unable to handle kernel NULL pointer dereference at virtual address 00000008 >> > pgd = ffffffc0f6c10000 >> > [00000008] *pgd=000000013b221003, *pud=000000013b221003, *pmd=0000000000000000a >> > 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 statp in __torture_print_stats(), if it is NULL, just >> > create a lock-torture-statistics message with zero statistic argument. >> >> It is keep to get the statistics printout at the end if with bad argument, >> what's your opinion about this version???
So we shouldn't be doing anything with statistics here in the first place, as it was a bogus argument. Instead, we should just exit with EINVAL. Something like the below does the trick for me.
Thanks, Davidlohr
----8<-------------------------------------------------------------------------- From: Davidlohr Bueso <dave@stgolabs.net> Subject: [PATCH] locktorture: Do not deal with statistics upon bogus input
Kefeng reported the following nil pointer dereference when passing an invalid lock type to torture.
[ 114.887250] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 [ 114.887254] IP: [<ffffffffa077808d>] __torture_print_stats+0x1d/0x100 [locktorture] ... [ 114.887315] CPU: 6 PID: 7080 Comm: modprobe Tainted: G I E 4.5.0-rc1-52.39-default+ #2 [ 114.887316] Hardware name: Dell Inc. PowerEdge R510/0W844P, BIOS 1.3.6 05/25/2010 [ 114.887318] task: ffff880196610040 ti: ffff8801a8ca4000 task.ti: ffff8801a8ca4000 [ 114.887322] RIP: 0010:[<ffffffffa077808d>] [<ffffffffa077808d>] __torture_print_stats+0x1d/0x100 [locktorture] [ 114.887324] RSP: 0018:ffff8801a8ca7c38 EFLAGS: 00010202 [ 114.887326] RAX: 0000000000000000 RBX: 0000000000004000 RCX: ffffea0006a12b20 [ 114.887327] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff8801aa7bc000 [ 114.887328] RBP: ffff8801a8ca7c58 R08: 0000000000000004 R09: ffff8801aa7bc000 [ 114.887329] R10: 000000000001c1a8 R11: ffff8801afc77d10 R12: 0000000000004000 [ 114.887330] R13: ffff8801aa7bc000 R14: ffff8801aaa312b0 R15: ffff8801a8ca7ec8 [ 114.887332] FS: 00007f13182bc700(0000) GS:ffff8801afc60000(0000) knlGS:0000000000000000 [ 114.887333] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [ 114.887334] CR2: 0000000000000008 CR3: 0000000196eb1000 CR4: 00000000000006e0 [ 114.887335] Stack: [ 114.887337] 0000000000004000 ffffffffa077a000 ffff8801aaa312b0 0000000000004000 [ 114.887339] ffff8801a8ca7c80 ffffffffa0778ba1 0000000000000048 00000000ffffffff [ 114.887341] ffffffffa077a000 ffff8801a8ca7cd0 ffffffffa0778d6d ffff8801a8ca7cb0 [ 114.887341] Call Trace: [ 114.887347] [<ffffffffa0778ba1>] lock_torture_stats_print+0x71/0x100 [locktorture] [ 114.887351] [<ffffffffa0778d6d>] lock_torture_cleanup+0xcd/0x28b [locktorture] [ 114.887358] [<ffffffff81084f6c>] ? blocking_notifier_chain_register+0x5c/0xa0 [ 114.887361] [<ffffffff81085f88>] ? register_reboot_notifier+0x18/0x20 [ 114.887365] [<ffffffffa077e0fa>] lock_torture_init+0xfa/0x1000 [locktorture] [ 114.887367] [<ffffffffa077e000>] ? 0xffffffffa077e000 [ 114.887372] [<ffffffff810003dd>] do_one_initcall+0xad/0x1e0
There is no reason for us to be calling into statistics logic as we should just return to the user:
lock-torture: invalid torture type: "mutex"
Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com> Signed-off-by: Davidlohr Bueso <dbueso@suse.de> --- kernel/locking/locktorture.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c index 8ef1919..f613fff 100644 --- a/kernel/locking/locktorture.c +++ b/kernel/locking/locktorture.c @@ -811,8 +811,9 @@ static int __init lock_torture_init(void) for (i = 0; i < ARRAY_SIZE(torture_ops); i++) pr_alert(" %s", torture_ops[i]->name); pr_alert("\n"); - firsterr = -EINVAL; - goto unwind; + + torture_init_end(); + return -EINVAL; } if (cxt.cur_ops->init) cxt.cur_ops->init(); -- 2.1.4
| |