Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid

[Davidlohr Bueso <dave@stgolabs.net>]

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