From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758006AbcATTej (ORCPT ); Wed, 20 Jan 2016 14:34:39 -0500 Received: from e37.co.us.ibm.com ([32.97.110.158]:44819 "EHLO e37.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751112AbcATTeg (ORCPT ); Wed, 20 Jan 2016 14:34:36 -0500 X-IBM-Helo: d03dlp02.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Wed, 20 Jan 2016 11:34:35 -0800 From: "Paul E. McKenney" To: Kefeng Wang Cc: Ingo Molnar , Peter Zijlstra , linux-kernel@vger.kernel.org, guohanjun@huawei.com Subject: Re: [PATCH] locktorture: Fix NULL pointer when torture_type is invalid Message-ID: <20160120193435.GK3818@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <1452650782-15099-1-git-send-email-wangkefeng.wang@huawei.com> <20160113162733.GO3818@linux.vnet.ibm.com> <5696FBFC.40702@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5696FBFC.40702@huawei.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16012019-0025-0000-0000-00002095A7ED Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 : [] 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 stats_task in lock_torture_cleanup() to guard not into > >> a wrong path. > >> > >> Signed-off-by: Kefeng Wang > > > > 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