Message-ID: <CAHp75Vf+zzVyVZ=fcXgdCMsp1MZdHmJ1aBTKFE4jHai2pWi27g@mail.gmail.com>
Date: Fri, 1 Mar 2024 13:20:41 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Andy Shevchenko <andy@...nel.org>, Kent Overstreet <kent.overstreet@...ux.dev>, 
	linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit

On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@...omium.org> wrote:
>
> Convert test-string_helpers.c to KUnit so it can be easily run with
> everything else.

..

> -#include <linux/array_size.h>
>  #include <linux/init.h>
> +#include <kunit/test.h>

I know the order is broken here, but don't make it worse, please. And
stick with one schema where to put kunit/test.h always before
everything else and somewhere else (like always after linux/*).

> +#include <linux/array_size.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>

..

> +static void test_string_check_buf(struct kunit *test,
> +                                 const char *name, unsigned int flags,
> +                                 char *in, size_t p,
> +                                 char *out_real, size_t q_real,
> +                                 char *out_test, size_t q_test)
>  {
> -       if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> -               return true;
> +       int result;
> +
> +       KUNIT_EXPECT_EQ(test, q_real, q_test);

This needs a message.

> +       result = memcmp(out_test, out_real, q_test);

> +       KUNIT_EXPECT_EQ(test, 0, result);

Why do we need this assertion? We have a dump below to show what's wrong.

> +       if (q_real == q_test && result == 0)
> +               return;

I'm not sure this is an equivalent change. IIRC KUnit assertions do
not continue on failure. (Long time last time I run KUnit test)

>
>         pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
>
> @@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *name, unsigned int flags,
>                        out_test, q_test, true);
>         print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
>                        out_real, q_real, true);
> -
> -       return false;
>  }

..

> +static void
> +test_string_escape_overflow(struct kunit *test,
> +                           const char *in, int p, unsigned int flags, const char *esc,
>                             int q_test, const char *name)
>  {
>         int q_real;
>
>         q_real = string_escape_mem(in, p, NULL, 0, flags, esc);
> -       if (q_real != q_test)
> -               pr_warn("Test '%s' failed: flags = %#x, osz = 0, expected %d, got %d\n",
> -                       name, flags, q_test, q_real);
> +       KUNIT_EXPECT_EQ(test, q_real, q_test);

You killed the message, not good.

>  }

..

> +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2)      \
> +       do {                                                                     \
> +               BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf);    \
> +               BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf);     \

No analogous assertions in KUnit?

> +               __test_string_get_size(test, (size), (blk_size), (exp_result10), \
> +                                      (exp_result2));                           \
>         } while (0)

..

>  {
> -       if (!memcmp(res, exp, strlen(exp) + 1))
> +       int result = memcmp(res, exp, strlen(exp) + 1);

> +       KUNIT_EXPECT_EQ(test, 0, result);

As per above, what's the added value of this assertion?

> +       if (!result)
>                 return;

..

> @@ -590,65 +604,68 @@ static void __init test_string_upper_lower(void)
>         for (i = 0; i < ARRAY_SIZE(strings_upper); i++) {
>                 const char *s = strings_upper[i].in;
>                 int len = strlen(strings_upper[i].in) + 1;
> +               int result;
>
>                 dst = kmalloc(len, GFP_KERNEL);
> -               if (!dst)
> -                       return;
> +               KUNIT_ASSERT_NOT_NULL(test, dst);
>
>                 string_upper(dst, s);
> -               if (memcmp(dst, strings_upper[i].out, len)) {
> +               result = memcmp(dst, strings_upper[i].out, len);
> +               KUNIT_EXPECT_EQ(test, 0, result);

Ditto.

> +               if (result)
>                         pr_warn("Test 'string_upper' failed : expected %s, got %s!\n",
>                                 strings_upper[i].out, dst);
> -                       kfree(dst);
> -                       return;
> -               }
>                 kfree(dst);
>         }
>
>         for (i = 0; i < ARRAY_SIZE(strings_lower); i++) {
>                 const char *s = strings_lower[i].in;
>                 int len = strlen(strings_lower[i].in) + 1;
> +               int result;
>
>                 dst = kmalloc(len, GFP_KERNEL);
> -               if (!dst)
> -                       return;
> +               KUNIT_ASSERT_NOT_NULL(test, dst);
>
>                 string_lower(dst, s);
> -               if (memcmp(dst, strings_lower[i].out, len)) {
> +               result = memcmp(dst, strings_lower[i].out, len);
> +               KUNIT_EXPECT_EQ(test, 0, result);

Ditto.

> +               if (result)
>                         pr_warn("Test 'string_lower failed : : expected %s, got %s!\n",
>                                 strings_lower[i].out, dst);
> -                       kfree(dst);
> -                       return;
> -               }
>                 kfree(dst);
>         }
>  }


-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists