Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757221AbcCCIgg (ORCPT ); Thu, 3 Mar 2016 03:36:36 -0500 Received: from mx2.suse.de ([195.135.220.15]:41630 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757091AbcCCIgf (ORCPT ); Thu, 3 Mar 2016 03:36:35 -0500 Date: Thu, 3 Mar 2016 00:36:26 -0800 From: Davidlohr Bueso To: Kefeng Wang Cc: paulmck@linux.vnet.ibm.com, 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: <20160303083626.GA10957@linux-uzut.site> References: <20160131221736.GB16147@linux-uzut.site> <56AEC21A.5010107@huawei.com> <20160201030235.GC16147@linux-uzut.site> <56AED0C7.7050505@huawei.com> <20160202064635.GH6719@linux.vnet.ibm.com> <20160203002331.GA3385@linux-uzut.site> <20160302195543.GA12593@linux-uzut.site> <20160302211216.GC3577@linux.vnet.ibm.com> <56D79566.7010302@huawei.com> <56D7BE33.5010605@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <56D7BE33.5010605@huawei.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 Content-Length: 4025 Lines: 116 On Thu, 03 Mar 2016, Kefeng Wang wrote: >Even if we merge Davidlohr's patch, I think we still need my v2 patch, >here is a scene, >---------- >cxt.lwsa = kmalloc(sizeof(*cxt.lwsa) * cxt.nrealwriters_stress, GFP_KERNEL); >if (cxt.lwsa == NULL) { > goto unwind; >} > >or > >cxt.lrsa = kmalloc(sizeof(*cxt.lrsa) * cxt.nrealreaders_stress, GFP_KERNEL); >if (cxt.lrsa == NULL) { > VERBOSE_TOROUT_STRING("cxt.lrsa: Out of memory"); > firsterr = -ENOMEM; > kfree(cxt.lwsa); > goto unwind; >} >---------- >we will get cxt.lwsa = NULL, and go to cleanup, then in > >static void __torture_print_stats(char *page, > struct lock_stress_stats *statp, bool write) >{ > bool fail = 0; > int i, n_stress; > long max = 0; > long min = statp[0].n_lock_acquired; // here, *we will meet NULL pointer dereference* > >} You are correct here, although very unlikely to hit a ENOMEM path, and because of the nature of the module, you have bigger problems than this anyway. That said, yes my patch only addresses this partially. >and my patch v2 solve this issue too, so it is still needed. But your patch is still too ad-hoc and still does not strike me to be the correct way of dealing with the issue due to the already mentioned issues. Lets instead think about how we call lock_torture_cleanup(). Callers are failed paths when loading the module, timed-shutdown and module_exit. All of these assume there is at least the writer stats existing (lwsa). That's actually why we have the "Start of test" shown immediately after doing basic checks. In my patch I had just assumed this was limited to sanitizing parameters, and overlooked mem allocation bits. The below should take care of both issues, what do you think? Thanks, Davidlohr <8------------------------------------------------------------------------- Subject: [PATCH] locktorture: Fix nil pointer dereferencing for cleanup paths It has been found that paths that invoke cleanups through lock_torture_cleanup() can incur in nil pointer dereferencing bugs during the statistics printing phase. This is mainly because we should not be calling into statistics before we are sure things have been setup correctly. Specifically, early checks (and the need for handling this in the cleanup call) only include parameter checks and basic statistics allocation. Once we start write/read kthreads we then consider the test as started. As such, update the func in question to check for cxt.lwsa writer stats, if not set, we either have a bogus parameter or ENOMEM situation and therefore only need to deal with general torture calls. Signed-off-by: Davidlohr Bueso --- XXX: while looking at the code, do we need at least a stat_interval > 0 check before stopping the lock_torture_stats kthread? kernel/locking/locktorture.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c index 8ef1919..1942848 100644 --- a/kernel/locking/locktorture.c +++ b/kernel/locking/locktorture.c @@ -748,6 +748,15 @@ static void lock_torture_cleanup(void) if (torture_cleanup_begin()) return; + /* + * Indicates early cleanup, meaning that the test has not run, + * such as when passing bogus args when loading the module. As + * such, only perform the underlying torture-specific cleanups, + * and avoid anything related to locktorture. + */ + if (!cxt.lwsa) + goto end; + if (writer_tasks) { for (i = 0; i < cxt.nrealwriters_stress; i++) torture_stop_kthread(lock_torture_writer, @@ -776,6 +785,7 @@ static void lock_torture_cleanup(void) else lock_torture_print_module_parms(cxt.cur_ops, "End of test: SUCCESS"); +end: torture_cleanup_end(); } @@ -878,6 +888,7 @@ static int __init lock_torture_init(void) cxt.lrsa[i].n_lock_acquired = 0; } } + lock_torture_print_module_parms(cxt.cur_ops, "Start of test"); /* Prepare torture context. */ -- 2.1.4