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

From: Kefeng Wang
Date: Sun Jan 31 2016 - 21:26:24 EST

Next message: Linus Torvalds: "Linux 4.5-rc2"
Previous message: Chen Yu: "[PATCH][v2] ACPI: Do not report _OSI("Darwin") when acpi_osi=!Darwin provided"
In reply to: Davidlohr Bueso: "Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid"
Next in thread: Davidlohr Bueso: "Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Linus Torvalds: "Linux 4.5-rc2"
Previous message: Chen Yu: "[PATCH][v2] ACPI: Do not report _OSI("Darwin") when acpi_osi=!Darwin provided"
In reply to: Davidlohr Bueso: "Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid"
Next in thread: Davidlohr Bueso: "Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is invalid"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]