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

[Paul E. McKenney]

On Thu, Jan 14, 2016 at 09:38:04AM +0800, Kefeng Wang wrote:
> Hi paul,
> 
> On 2016/1/14 0:27, Paul E. McKenney wrote:
> > On Wed, Jan 13, 2016 at 10:06:22AM +0800, Kefeng Wang wrote:
> >> Insmod locktorture with torture_type=mutex will lead to crash,
> >>
> >> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> >> pgd = ffffffc0f6c10000
> >> [00000008] *pgd=000000013b221003, *pud=000000013b221003, *pmd=0000000000000000
> >> 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 stats_task in lock_torture_cleanup() to guard not into
> >> a wrong path.
> >>
> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> > 
> > Good catch!  One question below...
> > 
> >> ---
> >>  kernel/locking/locktorture.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> >> index 8ef1919..1afa8ad 100644
> >> --- a/kernel/locking/locktorture.c
> >> +++ b/kernel/locking/locktorture.c
> >> @@ -764,8 +764,10 @@ static void lock_torture_cleanup(void)
> >>  		reader_tasks = NULL;
> >>  	}
> >>
> >> -	torture_stop_kthread(lock_torture_stats, stats_task);
> >> -	lock_torture_stats_print();  /* -After- the stats thread is stopped! */
> >> +	if (stats_task) {
> >> +		torture_stop_kthread(lock_torture_stats, stats_task);
> >> +		lock_torture_stats_print();  /* -After- the stats thread is stopped! */
> >> +	}
> > 
> > Does it work to have the following?
> > 
> > +	if (stats_task)
> > +		torture_stop_kthread(lock_torture_stats, stats_task);
> > +	lock_torture_stats_print();  /* -After- the stats thread is stopped! */
> > 
> > We need to print the statistics at the end of the run, even if we chose
> > not to print them periodically throughout the run.
> > 
> 
> No, the pc is in __torture_print_stats(char *page, struct lock_stress_stats *statp, bool write),
> that is 'long min = statp[0].n_lock_acquired;', if the torture_type is invalid, the statp is NULL,
> because the cxt.lwsa and cxt.lwsa/cxt.lrsa are not initialized in lock_torture_init().

Good point, thank you for checking!

The reason that we don't have this problem in rcutorture is the lack
of dynamically allocated statistics.

It is really nice to get the statistics printout at the end, so it
would be good for locktorture to keep it as well.  Having that
helps prevent false-positive test passes due to bugs that skip
lock acquisitions and the like.

So would it make sense to unconditionally allocate and free the
memory, regardless of whether or not there was a stats kthread?

							Thanx, Paul