Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid
From: Davidlohr Bueso
Date: Sun Jan 31 2016 - 17:17:51 EST
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@xxxxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
---
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