Re: [PATCH v2] list: test: potential dereference of null pointer

[Daniel Latypov <dlatypov@xxxxxxxxxx>]

On Fri, Sep 22, 2023 at 4:40 AM Ma Ke <make_ruc2021@xxxxxxx> wrote:
>
> To avoid the failure of alloc, we could check the return value of
> kmalloc() and kzalloc().

Thanks, that's a good point, some suggestions below.
And also a question for David Gow whenever he sees this.

>
> Signed-off-by: Ma Ke <make_ruc2021@xxxxxxx>
> ---
>  lib/list-test.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/lib/list-test.c b/lib/list-test.c
> index 0cc27de9cec8..70e898976dbf 100644
> --- a/lib/list-test.c
> +++ b/lib/list-test.c
> @@ -27,9 +27,18 @@ static void list_test_list_init(struct kunit *test)
>         INIT_LIST_HEAD(&list2);
>
>         list4 = kzalloc(sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL);
> +       if (!list4) {
> +               KUNIT_FAIL(test, "Initialising list4 failed.\n");
> +               return;
> +       }

Note: we can replace this with a one-liner
  KUNIT_ASSERT_NOT_NULL(test, list4);

>         INIT_LIST_HEAD(list4);
>
>         list5 = kmalloc(sizeof(*list5), GFP_KERNEL | __GFP_NOFAIL);
> +       if (!list5) {
> +               kfree(list4);

We can also replace this check with the one-liner
  KUNIT_ASSERT_NOT_NULL(test, list5);

But we'd need to migrate the kzalloc() call to use kunit_kzalloc():
  kunit_kzalloc(test, sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL);
that way we don't have to manually free list4.

I'm not sure why the original version didn't use the kunit helpers to
begin with.
Perhaps David would remember.

A quick lazy ^F over the original patch didn't find anything afaict,
https://lore.kernel.org/linux-kselftest/a127aeaa-e5ba-2d8d-0894-936e05637508@xxxxxxxxxx/T/#u


> +               KUNIT_FAIL(test, "Initialising list5 failed.\n");
> +               return;
> +       }

Daniel