From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757564AbcAaWRr (ORCPT ); Sun, 31 Jan 2016 17:17:47 -0500 Received: from mx2.suse.de ([195.135.220.15]:37056 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757466AbcAaWRp convert rfc822-to-8bit (ORCPT ); Sun, 31 Jan 2016 17:17:45 -0500 Date: Sun, 31 Jan 2016 14:17:36 -0800 From: Davidlohr Bueso To: "Paul E. McKenney" Cc: Kefeng Wang , linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, Josh Triplett , "Guohanjun (Hanjun Guo)" Subject: Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid Message-ID: <20160131221736.GB16147@linux-uzut.site> References: <1453955159-23216-1-git-send-email-wangkefeng.wang@huawei.com> <56AC2421.7020006@huawei.com> <20160131002721.GI6719@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: <20160131002721.GI6719@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 : [] lr : [] pstate: 60000145 >> > sp : ffffffc0fa93bb20 >> > [snip...] >> > Call trace: >> > [] __torture_print_stats+0x18/0x180 [locktorture] >> > [] lock_torture_stats_print+0x68/0x110 [locktorture] >> > [] lock_torture_cleanup+0xc4/0x278 [locktorture] >> > [] lock_torture_init+0x144/0x5b0 [locktorture] >> > [] do_one_initcall+0x94/0x1a0 >> > [] do_init_module+0x60/0x1c8 >> > [] load_module+0x1880/0x1c9c >> > [] SyS_finit_module+0x7c/0x88 >> > [] 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 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: [] __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:[] [] __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] [] lock_torture_stats_print+0x71/0x100 [locktorture] [ 114.887351] [] lock_torture_cleanup+0xcd/0x28b [locktorture] [ 114.887358] [] ? blocking_notifier_chain_register+0x5c/0xa0 [ 114.887361] [] ? register_reboot_notifier+0x18/0x20 [ 114.887365] [] lock_torture_init+0xfa/0x1000 [locktorture] [ 114.887367] [] ? 0xffffffffa077e000 [ 114.887372] [] 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 Signed-off-by: Davidlohr Bueso --- 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