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

From: Kefeng Wang
Date: Mon Mar 07 2016 - 02:05:39 EST




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@xxxxxxxxxx>
> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
> ---
> 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. */