[v2,RESEND] kunit: debugfs: Handle errors from alloc_string_stream()

In kunit_debugfs_create_suite() give up and skip creating the debugfs
file if any of the alloc_string_stream() calls return an error or NULL.
Only put a value in the log pointer of kunit_suite and kunit_test if it
is a valid pointer to a log.

This prevents the potential invalid dereference reported by smatch:

 lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log'
	dereferencing possible ERR_PTR()
 lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log'
	dereferencing possible ERR_PTR()

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 05e2006ce493 ("kunit: Use string_stream for test log")
---
 lib/kunit/debugfs.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

On Mon, Oct 30, 2023 at 6:47 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> In kunit_debugfs_create_suite() give up and skip creating the debugfs
> file if any of the alloc_string_stream() calls return an error or NULL.
> Only put a value in the log pointer of kunit_suite and kunit_test if it
> is a valid pointer to a log.
>
> This prevents the potential invalid dereference reported by smatch:
>
>  lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log'
>         dereferencing possible ERR_PTR()
>  lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log'
>         dereferencing possible ERR_PTR()

Hello!

Thanks for sending the re-sends of these patches! This patch also
looks good to me! I have one comment below but I would still be happy
with the patch as is.

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!
-Rae

>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 05e2006ce493 ("kunit: Use string_stream for test log")
> ---
>  lib/kunit/debugfs.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> index 270d185737e6..9d167adfa746 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -109,14 +109,28 @@ static const struct file_operations debugfs_results_fops = {
>  void kunit_debugfs_create_suite(struct kunit_suite *suite)
>  {
>         struct kunit_case *test_case;
> +       struct string_stream *stream;
>
> -       /* Allocate logs before creating debugfs representation. */
> -       suite->log = alloc_string_stream(GFP_KERNEL);
> -       string_stream_set_append_newlines(suite->log, true);
> +       /*
> +        * Allocate logs before creating debugfs representation.
> +        * The suite->log and test_case->log pointer are expected to be NULL
> +        * if there isn't a log, so only set it if the log stream was created
> +        * successfully.
> +        */

I like this new comment. Thanks!

> +       stream = alloc_string_stream(GFP_KERNEL);
> +       if (IS_ERR_OR_NULL(stream))

In response to Dan Carpenter's comment from the last version, I see
the benefits of changing IS_ERR_OR_NULL() to IS_ERR() instead because
"stream" will not be NULL. This would then also be the same as the
check in kunit_alloc_string_stream.

However, I also see the benefit of checking for NULL just in case anyways.

I'm overall happy with either solution but just wanted to bring this up.

> +               return;
> +
> +       string_stream_set_append_newlines(stream, true);
> +       suite->log = stream;
>
>         kunit_suite_for_each_test_case(suite, test_case) {
> -               test_case->log = alloc_string_stream(GFP_KERNEL);
> -               string_stream_set_append_newlines(test_case->log, true);
> +               stream = alloc_string_stream(GFP_KERNEL);
> +               if (IS_ERR_OR_NULL(stream))
> +                       goto err;
> +
> +               string_stream_set_append_newlines(stream, true);
> +               test_case->log = stream;
>         }
>
>         suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
> @@ -124,6 +138,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite)
>         debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444,
>                             suite->debugfs,
>                             suite, &debugfs_results_fops);
> +       return;
> +
> +err:
> +       string_stream_destroy(suite->log);
> +       kunit_suite_for_each_test_case(suite, test_case)
> +               string_stream_destroy(test_case->log);
>  }
>
>  void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
> --
> 2.30.2
>

On Mon, 30 Oct 2023 at 18:47, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> In kunit_debugfs_create_suite() give up and skip creating the debugfs
> file if any of the alloc_string_stream() calls return an error or NULL.
> Only put a value in the log pointer of kunit_suite and kunit_test if it
> is a valid pointer to a log.
>
> This prevents the potential invalid dereference reported by smatch:
>
>  lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log'
>         dereferencing possible ERR_PTR()
>  lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log'
>         dereferencing possible ERR_PTR()
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 05e2006ce493 ("kunit: Use string_stream for test log")
> ---


Thanks for fixing all the nasty C error handling.

Closes: https://groups.google.com/g/kunit-dev/c/sf6MsFzeEV4
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

On Thu, Nov 30, 2023 at 04:11:18PM -0500, Rae Moar wrote:
> > +       stream = alloc_string_stream(GFP_KERNEL);
> > +       if (IS_ERR_OR_NULL(stream))
> 
> In response to Dan Carpenter's comment from the last version, I see
> the benefits of changing IS_ERR_OR_NULL() to IS_ERR() instead because
> "stream" will not be NULL. This would then also be the same as the
> check in kunit_alloc_string_stream.
> 
> However, I also see the benefit of checking for NULL just in case anyways.
> 

Returning NULL in alloc_string_stream() is a bug.  Checking for NULL is
a work around for bugs.  There are basically two times where it can
be valid to work around bugs like this instead of fixing them.  1)  When
the function is implemented by over 10 different driver authors.  In
that case you can guarantee that at least one of them is going to do the
wrong thing.  There are between 2-5 places which do this in the kernel.
2)  If it's a API that used to return NULL and it's changed to returning
error pointers.  I've never seen anyone do this, but I've proposed it as
a solution to make backporting easier.

regards,
dan carpenter