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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

Hi Davidlohr,

On 2016/2/1 6:17, Davidlohr Bueso wrote:
> 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.
> 
[snip...]
> 
> 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.
> 

Yes, it works, but what you are doing is to revert commit a36a99618b1adb2d6ca0b7e08e3a656a04e477fe
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Sun Aug 30 20:01:48 2015 -0700

    locktorture: Fix module unwind when bad torture_type specified

    The locktorture module has a list of torture types, and specifying
    a type not on this list is supposed to cleanly fail the module load.
    Unfortunately, the "fail" happens without the "cleanly".  This commit
    therefore adds the needed clean-up after an incorrect torture_type.


If we revert it, the cleanup is not completely after insmod lockture with bad torture_type,
we can't insmod lockture with correct argument anymore, we will get following print,

-bash-4.3# insmod locktorture.ko torture_type=mutex
[  340.872178] lock-torture: invalid torture type: "mutex"
[  340.873185] lock-torture types:
[  340.873665]  lock_busted spin_lock
[  340.874182]  spin_lock_irq rw_lock
[  340.883853]  rw_lock_irq mutex_lock
[  340.884238]  rtmutex_lock rwsem_lock
[  340.884640]  percpu_rwsem_lock[  340.884941]
insmod: ERROR: could not insert module locktorture.ko: Invalid parameters
-bash-4.3# lsmod
Module                  Size  Used by
torture                17672  0
-bash-4.3# insmod locktorture.ko torture_type=mutex_lock
[  356.796114] torture_init_begin: refusing mutex_lock init: mutex running
insmod: ERROR: could not insert module locktorture.ko: Device or resource busy

And what Paul wanted is something that would print the full statistics
at the end regardless of the periodic statistics.

I prefer my version 2, here is some log with my patch v2, it is keep consistent
with rcutorture.
-------------------------------------------------------
-bash-4.3# insmod locktorture.ko torture_type=mutex
[  190.845067] lock-torture: invalid torture type: "mutex"
[  190.845748] lock-torture types:
[  190.846099]  lock_busted spin_lock
[  190.863211]  spin_lock_irq rw_lock
[  190.863668]  rw_lock_irq mutex_lock
[  190.864049]  rtmutex_lock rwsem_lock
[  190.864390]  percpu_rwsem_lock[  190.864686]
[  190.865662] Writes:  Total: 0  Max/Min: 0/0   Fail: 0
[  190.866218] Reads :  Total: 0  Max/Min: 0/0   Fail: 0
[  190.875071] mutex-torture:--- End of test: SUCCESS: nwriters_stress=0 nreaders_stress=0 stat_interval=60 verbose=1 shuffle_interval=3 stutter=5 shutdown_secs=0 onoff_interval=0 onoff_holdoff=0
insmod: ERROR: could not insert module locktorture.ko: Invalid parameters

-bash-4.3# insmod locktorture.ko torture_type=mutex_lock
[  201.440136] mutex_lock-torture:--- Start of test: nwriters_stress=2 nreaders_stress=0 stat_interval=60 verbose=1 shuffle_interval=3 stutter=5 shutdown_secs=0 onoff_interval=0 onoff_holdoff=0
[  201.441666] mutex_lock-torture: Creating torture_shuffle task
[  201.447173] mutex_lock-torture: Creating torture_stutter task
[  201.448124] mutex_lock-torture: torture_shuffle task started
[  201.452483] mutex_lock-torture: Creating lock_torture_writer task
[  201.453670] mutex_lock-torture: torture_stutter task started
[  201.459217] mutex_lock-torture: Creating lock_torture_writer task
[  201.460204] mutex_lock-torture: lock_torture_writer task started
[  201.460976] mutex_lock-torture: Creating lock_torture_stats task
[  201.461668] mutex_lock-torture: lock_torture_writer task started
[  201.471916] mutex_lock-torture: lock_torture_stats task started
-bash-4.3# rmmod locktorture.ko
[  209.294964] mutex_lock-torture: Stopping torture_shuffle task
[  209.295874] mutex_lock-torture: Stopping torture_shuffle
[  209.296847] mutex_lock-torture: Stopping torture_stutter task
[  209.297458] mutex_lock-torture: Stopping torture_stutter
[  209.301694] mutex_lock-torture: Stopping lock_torture_writer task
[  209.318665] mutex_lock-torture: Stopping lock_torture_writer
[  209.319522] mutex_lock-torture: Stopping lock_torture_writer task
[  209.341051] mutex_lock-torture: Stopping lock_torture_writer
[  209.341864] mutex_lock-torture: Stopping lock_torture_stats task
[  209.344949] Writes:  Total: 378  Max/Min: 0/0   Fail: 0
[  209.345617] mutex_lock-torture: Stopping lock_torture_stats
[  209.346817] Writes:  Total: 378  Max/Min: 0/0   Fail: 0
[  209.347389] mutex_lock-torture:--- End of test: SUCCESS: nwriters_stress=2 nreaders_stress=0 stat_interval=60 verbose=1 shuffle_interval=3 stutter=5 shutdown_secs=0 onoff_interval=0 onoff_holdoff=0
-------------------------------------------------------

Thanks,
Kefeng

> 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();