Received-SPF: pass (google.com: domain of linux-kernel+bounces-88330-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240301002416.it.092-kees@kernel.org> <20240301002650.1544829-2-keescook@chromium.org>
In-Reply-To: <20240301002650.1544829-2-keescook@chromium.org>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Fri, 1 Mar 2024 13:20:41 +0200
Message-ID: <CAHp75Vf+zzVyVZ=fcXgdCMsp1MZdHmJ1aBTKFE4jHai2pWi27g@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit
To: Kees Cook <keescook@chromium.org>
Cc: Andy Shevchenko <andy@kernel.org>, Kent Overstreet <kent.overstreet@linux.dev>, 
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 1, 2024 at 2:26=E2=80=AFAM Kees Cook <keescook@chromium.org> wr=
ote:
>
> 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 =3D=3D 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 =3D 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 =3D=3D q_test && result =3D=3D 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 =3D %#x\n", name, flags);
>
> @@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *na=
me, 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, co=
nst char *esc,
>                             int q_test, const char *name)
>  {
>         int q_real;
>
>         q_real =3D string_escape_mem(in, p, NULL, 0, flags, esc);
> -       if (q_real !=3D q_test)
> -               pr_warn("Test '%s' failed: flags =3D %#x, osz =3D 0, expe=
cted %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_resul=
t2)      \
> +       do {                                                             =
        \
> +               BUILD_BUG_ON(sizeof(exp_result10) >=3D string_get_size_ma=
xbuf);    \
> +               BUILD_BUG_ON(sizeof(exp_result2) >=3D string_get_size_max=
buf);     \

No analogous assertions in KUnit?

> +               __test_string_get_size(test, (size), (blk_size), (exp_res=
ult10), \
> +                                      (exp_result2));                   =
        \
>         } while (0)

..

>  {
> -       if (!memcmp(res, exp, strlen(exp) + 1))
> +       int result =3D 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 =3D 0; i < ARRAY_SIZE(strings_upper); i++) {
>                 const char *s =3D strings_upper[i].in;
>                 int len =3D strlen(strings_upper[i].in) + 1;
> +               int result;
>
>                 dst =3D 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 =3D 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 =3D 0; i < ARRAY_SIZE(strings_lower); i++) {
>                 const char *s =3D strings_lower[i].in;
>                 int len =3D strlen(strings_lower[i].in) + 1;
> +               int result;
>
>                 dst =3D 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 =3D 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);
>         }
>  }


--=20
With Best Regards,
Andy Shevchenko