Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1348440 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Davidlohr Bueso <dave@stgolabs.net> - 2016-03-02 21:00 +0100
    Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-03-02 22:20 +0100
      Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Kefeng Wang <wangkefeng.wang@huawei.com> - 2016-03-03 02:50 +0100
        Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Kefeng Wang <wangkefeng.wang@huawei.com> - 2016-03-03 05:50 +0100
          Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Davidlohr Bueso <dave@stgolabs.net> - 2016-03-03 09:40 +0100
        Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-03-03 15:20 +0100

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

On Tue, 02 Feb 2016, Davidlohr Bueso wrote:

I've just hit this issue myself and remembered this thread :)

Paul, folks, does the below patch look reasonable to you? If so
I can properly resend. thanks.

>On Mon, 01 Feb 2016, Paul E. McKenney wrote:
>
>>On Mon, Feb 01, 2016 at 11:28:07AM +0800, Kefeng Wang wrote:
>
>>>Just like I mentioned before, keep consistent with rcutorture???
>
>Because rcutorture does it doesn't mean locktorture has to do it ;)
>In any case, I'd suggest the same be done for rcutorture.
>
>[...]
>
>>
>>Hmmm...   If nothing happened, then I agree that it makes sense not to
>>print any statistics.  But if some testing actually was carried out, then
>>we really need to print the statistics.
>
>Right, so how about the following? It introduces an early cleanup helper
>that all it does is do torture specific cleanups. I don't really love the
>begin/end calls there, but it's not the end of the world and it seems better
>than a more messier refactoring. ie, I had also considered adding an 'early'
>flag to lock_torture_cleanup() such that we can enable it for this bogus param
>scenario, but seems over complicating things and we also call it for such a
>small issue.
>
>Thanks,
>Davidlohr
>
>
>diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>index 8ef1919..05e2649 100644
>--- a/kernel/locking/locktorture.c
>+++ b/kernel/locking/locktorture.c
>@@ -741,6 +741,19 @@ lock_torture_print_module_parms(struct lock_torture_ops *cur_ops,
> 		 onoff_interval, onoff_holdoff);
> }
>+/*
>+ * 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.
>+ */
>+static inline void lock_torture_early_cleanup(void)
>+{
>+	if (torture_cleanup_begin())
>+		return;
>+	torture_cleanup_end();
>+}
>+
> static void lock_torture_cleanup(void)
> {
> 	int i;
>@@ -811,8 +824,10 @@ 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();
>+		lock_torture_early_cleanup();
>+		return -EINVAL;
> 	}
> 	if (cxt.cur_ops->init)
> 		cxt.cur_ops->init();

[toc] | [next] | [standalone]

#1348476

On Wed, Mar 02, 2016 at 11:55:43AM -0800, Davidlohr Bueso wrote:
> On Tue, 02 Feb 2016, Davidlohr Bueso wrote:
> 
> I've just hit this issue myself and remembered this thread :)
> 
> Paul, folks, does the below patch look reasonable to you? If so
> I can properly resend. thanks.

If it works for Kefeng Wang, I would be happy to take it.

							Thanx, Paul

> >On Mon, 01 Feb 2016, Paul E. McKenney wrote:
> >
> >>On Mon, Feb 01, 2016 at 11:28:07AM +0800, Kefeng Wang wrote:
> >
> >>>Just like I mentioned before, keep consistent with rcutorture???
> >
> >Because rcutorture does it doesn't mean locktorture has to do it ;)
> >In any case, I'd suggest the same be done for rcutorture.
> >
> >[...]
> >
> >>
> >>Hmmm...   If nothing happened, then I agree that it makes sense not to
> >>print any statistics.  But if some testing actually was carried out, then
> >>we really need to print the statistics.
> >
> >Right, so how about the following? It introduces an early cleanup helper
> >that all it does is do torture specific cleanups. I don't really love the
> >begin/end calls there, but it's not the end of the world and it seems better
> >than a more messier refactoring. ie, I had also considered adding an 'early'
> >flag to lock_torture_cleanup() such that we can enable it for this bogus param
> >scenario, but seems over complicating things and we also call it for such a
> >small issue.
> >
> >Thanks,
> >Davidlohr
> >
> >
> >diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> >index 8ef1919..05e2649 100644
> >--- a/kernel/locking/locktorture.c
> >+++ b/kernel/locking/locktorture.c
> >@@ -741,6 +741,19 @@ lock_torture_print_module_parms(struct lock_torture_ops *cur_ops,
> >		 onoff_interval, onoff_holdoff);
> >}
> >+/*
> >+ * 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.
> >+ */
> >+static inline void lock_torture_early_cleanup(void)
> >+{
> >+	if (torture_cleanup_begin())
> >+		return;
> >+	torture_cleanup_end();
> >+}
> >+
> >static void lock_torture_cleanup(void)
> >{
> >	int i;
> >@@ -811,8 +824,10 @@ 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();
> >+		lock_torture_early_cleanup();
> >+		return -EINVAL;
> >	}
> >	if (cxt.cur_ops->init)
> >		cxt.cur_ops->init();
> 

[toc] | [prev] | [next] | [standalone]

#1348682

On 2016/3/3 5:12, Paul E. McKenney wrote:
> On Wed, Mar 02, 2016 at 11:55:43AM -0800, Davidlohr Bueso wrote:
>> On Tue, 02 Feb 2016, Davidlohr Bueso wrote:
>>
>> I've just hit this issue myself and remembered this thread :)
>>
>> Paul, folks, does the below patch look reasonable to you? If so
>> I can properly resend. thanks.
> 
> If it works for Kefeng Wang, I would be happy to take it.

Yes, it works for me, tested on my board.


> 
> 							Thanx, Paul
> 

[toc] | [prev] | [next] | [standalone]

#1348784

Hi Davidlohr and Paul,

On 2016/3/3 9:37, Kefeng Wang wrote:
> 
> 
> On 2016/3/3 5:12, Paul E. McKenney wrote:
>> On Wed, Mar 02, 2016 at 11:55:43AM -0800, Davidlohr Bueso wrote:
>>> On Tue, 02 Feb 2016, Davidlohr Bueso wrote:
>>>
>>> I've just hit this issue myself and remembered this thread :)
>>>
>>> Paul, folks, does the below patch look reasonable to you? If so
>>> I can properly resend. thanks.
>>
>> If it works for Kefeng Wang, I would be happy to take it.
> 
> Yes, it works for me, tested on my board.
> 

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*

}

and my patch v2 solve this issue too, so it is still needed.

Thanks,
Kefeng


> 
>>
>> 							Thanx, Paul
>>
> 

[toc] | [prev] | [next] | [standalone]

#1348909

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 <dbueso@suse.de>
---
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

[toc] | [prev] | [next] | [standalone]

#1349202

On Thu, Mar 03, 2016 at 09:37:42AM +0800, Kefeng Wang wrote:
> 
> 
> On 2016/3/3 5:12, Paul E. McKenney wrote:
> > On Wed, Mar 02, 2016 at 11:55:43AM -0800, Davidlohr Bueso wrote:
> >> On Tue, 02 Feb 2016, Davidlohr Bueso wrote:
> >>
> >> I've just hit this issue myself and remembered this thread :)
> >>
> >> Paul, folks, does the below patch look reasonable to you? If so
> >> I can properly resend. thanks.
> > 
> > If it works for Kefeng Wang, I would be happy to take it.
> 
> Yes, it works for me, tested on my board.

Very good, thank you!

Davidlohr, please do send me a cleaned-up version of your patch.

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web