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

Groups > linux.kernel > #1351256 > 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 Kefeng Wang <wangkefeng.wang@huawei.com> - 2016-03-07 03:10 +0100
    Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Davidlohr Bueso <dave@stgolabs.net> - 2016-03-07 06:50 +0100
      Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Kefeng Wang <wangkefeng.wang@huawei.com> - 2016-03-07 08:10 +0100
        Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-03-07 16:40 +0100
          Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid Kefeng Wang <wangkefeng.wang@huawei.com> - 2016-03-08 03:20 +0100
            Re: [PATCH v2] locktorture: Fix NULL pointer when torture_type is  invalid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-03-08 21:00 +0100

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

On 2016/3/3 16:36, Davidlohr Bueso wrote:
> On Thu, 03 Mar 2016, Kefeng Wang wrote:
> 

> 
> 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;

Sorry for the late response, the cxt.lrsa should be taken into account too.

> +
>      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. */

[toc] | [next] | [standalone]

#1351325

On Mon, 07 Mar 2016, Kefeng Wang wrote:
>On 2016/3/3 16:36, Davidlohr Bueso wrote:

>> +    /*
>> +     * 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;
>
>Sorry for the late response, the cxt.lrsa should be taken into account too.

I am taking it into account, note that we kfree lwsa if lrsa fails memory
allocation. Of course we should be defensive, so go ahead and explicitly set
it to nil. v2 below, otherwise same patch.

-----8<--------------------------
Subject: [PATCH v2] 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

Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
  kernel/locking/locktorture.c | 12 ++++++++++++
  1 file changed, 12 insertions(+)

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 8ef1919..b5bc243 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();
  }
  
@@ -870,6 +880,7 @@ static int __init lock_torture_init(void)
  			VERBOSE_TOROUT_STRING("cxt.lrsa: Out of memory");
  			firsterr = -ENOMEM;
  			kfree(cxt.lwsa);
+			cxt.lwsa = NULL;
  			goto unwind;
  		}
  
@@ -878,6 +889,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]

#1351333

On 2016/3/7 13:40, Davidlohr Bueso wrote:
> On Mon, 07 Mar 2016, Kefeng Wang wrote:
>> On 2016/3/3 16:36, Davidlohr Bueso wrote:
> 
>>> +    /*
>>> +     * 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;
>>
>> Sorry for the late response, the cxt.lrsa should be taken into account too.
> 
> I am taking it into account, note that we kfree lwsa if lrsa fails memory
> allocation. Of course we should be defensive, so go ahead and explicitly set
> it to nil. v2 below, otherwise same patch.

This one looks good, and tested on my board.


> 
> -----8<--------------------------
> Subject: [PATCH v2] 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
> 
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  kernel/locking/locktorture.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index 8ef1919..b5bc243 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();
>  }
>  
> @@ -870,6 +880,7 @@ static int __init lock_torture_init(void)
>              VERBOSE_TOROUT_STRING("cxt.lrsa: Out of memory");
>              firsterr = -ENOMEM;
>              kfree(cxt.lwsa);
> +            cxt.lwsa = NULL;
>              goto unwind;
>          }
>  
> @@ -878,6 +889,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. */

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

#1351722

On Mon, Mar 07, 2016 at 03:02:05PM +0800, Kefeng Wang wrote:
> 
> 
> On 2016/3/7 13:40, Davidlohr Bueso wrote:
> > On Mon, 07 Mar 2016, Kefeng Wang wrote:
> >> On 2016/3/3 16:36, Davidlohr Bueso wrote:
> > 
> >>> +    /*
> >>> +     * 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;
> >>
> >> Sorry for the late response, the cxt.lrsa should be taken into account too.
> > 
> > I am taking it into account, note that we kfree lwsa if lrsa fails memory
> > allocation. Of course we should be defensive, so go ahead and explicitly set
> > it to nil. v2 below, otherwise same patch.
> 
> This one looks good, and tested on my board.

Very good!  May we add your Tested-by?

							Thanx, Paul

> > -----8<--------------------------
> > Subject: [PATCH v2] 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
> > 
> > Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> > ---
> >  kernel/locking/locktorture.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> > index 8ef1919..b5bc243 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();
> >  }
> >  
> > @@ -870,6 +880,7 @@ static int __init lock_torture_init(void)
> >              VERBOSE_TOROUT_STRING("cxt.lrsa: Out of memory");
> >              firsterr = -ENOMEM;
> >              kfree(cxt.lwsa);
> > +            cxt.lwsa = NULL;
> >              goto unwind;
> >          }
> >  
> > @@ -878,6 +889,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. */
> 

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

#1352558

On 2016/3/7 21:37, Paul E. McKenney wrote:
> On Mon, Mar 07, 2016 at 03:02:05PM +0800, Kefeng Wang wrote:
>>
>>
>> On 2016/3/7 13:40, Davidlohr Bueso wrote:
>>> On Mon, 07 Mar 2016, Kefeng Wang wrote:
>>>> On 2016/3/3 16:36, Davidlohr Bueso wrote:
>>>
>>>>> +    /*
>>>>> +     * 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;
>>>>
>>>> Sorry for the late response, the cxt.lrsa should be taken into account too.
>>>
>>> I am taking it into account, note that we kfree lwsa if lrsa fails memory
>>> allocation. Of course we should be defensive, so go ahead and explicitly set
>>> it to nil. v2 below, otherwise same patch.
>>
>> This one looks good, and tested on my board.
> 
> Very good!  May we add your Tested-by?

Sure, please.

> 
> 							Thanx, Paul

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

#1353356

On Tue, Mar 08, 2016 at 10:10:52AM +0800, Kefeng Wang wrote:
> 
> 
> On 2016/3/7 21:37, Paul E. McKenney wrote:
> > On Mon, Mar 07, 2016 at 03:02:05PM +0800, Kefeng Wang wrote:
> >>
> >>
> >> On 2016/3/7 13:40, Davidlohr Bueso wrote:
> >>> On Mon, 07 Mar 2016, Kefeng Wang wrote:
> >>>> On 2016/3/3 16:36, Davidlohr Bueso wrote:
> >>>
> >>>>> +    /*
> >>>>> +     * 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;
> >>>>
> >>>> Sorry for the late response, the cxt.lrsa should be taken into account too.
> >>>
> >>> I am taking it into account, note that we kfree lwsa if lrsa fails memory
> >>> allocation. Of course we should be defensive, so go ahead and explicitly set
> >>> it to nil. v2 below, otherwise same patch.
> >>
> >> This one looks good, and tested on my board.
> > 
> > Very good!  May we add your Tested-by?
> 
> Sure, please.

Thank you, Kefeng!

Davidlohr, I tried applying your patches and got conflicts.  Could you
please port them to -rcu and send me clean versions?

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web