Re: [PATCH v3 7/7] UAPI: Introduce KUnit userspace compatibility

[David Gow]

On Mon, Feb 28, 2022 at 2:45 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> The original lib/test_stackinit.c, which exclusively tests toolchain
> features, was designed to also be built without the full Linux kernel
> sources so that compiler developers and distro maintainers had an easy
> way to check for toolchain behaviors. When it was ported to KUnit, this
> mode was removed to simplify the code.
>
> Add a small header that provides a minimally operational KUnit API that
> can allow unit tests that don't depend on kernel-specific behaviors
> to build and run strictly from userspace without kernel sources. Add
> userspace-build support back to the renamed lib/stackinit_kunit.c test.
>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> v1: https://lore.kernel.org/lkml/20220224055145.1853657-1-keescook@xxxxxxxxxxxx
> v2:
>  - split from stackinit_kunit.c refactoring patch
>  - add missing returns (Daniel)
>  - report expression mismatch in assert msg (Daniel)
>  - emulate kunit_test_suites() (Daniel)
>  - emit valid KTAP (David)
> ---

This looks pretty good on-the-whole to me, modulo one bug (test suites
being marked as SKIPPED instead of FAILED) below. And checkpatch being
grumpy.

I do like the idea of putting this in uapi/ -- it solves the problem
of distributing it quite elegantly, IMO.

-- David

>  include/uapi/misc/kunit.h | 181 ++++++++++++++++++++++++++++++++++++++
>  lib/stackinit_kunit.c     |  11 +++
>  2 files changed, 192 insertions(+)
>  create mode 100644 include/uapi/misc/kunit.h
>
> diff --git a/include/uapi/misc/kunit.h b/include/uapi/misc/kunit.h
> new file mode 100644
> index 000000000000..afdffda583ae
> --- /dev/null
> +++ b/include/uapi/misc/kunit.h
> @@ -0,0 +1,181 @@
> +#ifndef __UAPI_MISC_KUNIT_H__
> +#define __UAPI_MISC_KUNIT_H__
> +/*
> + * This is a light-weight userspace drop-in replacement for the in-kernel
> + * KUnit API. It seeks to implement a minimal subset of features so that
> + * a concisely written KUnit test can be made to run entirely in userspace
> + * when it doesn't actually depend on any real kernel internals.
> + *
> + * Additionally contains many refactored kernel-isms to support building
> + * and running in userspace without full kernel source.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdbool.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +
> +#define __user                 /**/
> +#define noinline               __attribute__((__noinline__))
> +#define __aligned(x)           __attribute__((__aligned__(x)))
> +#ifdef __clang__
> +# define __compiletime_error(message) /**/
> +#else
> +# define __compiletime_error(message) __attribute__((__error__(message)))
> +#endif
> +#define __compiletime_assert(condition, msg, prefix, suffix)           \
> +       do {                                                            \
> +               extern void prefix ## suffix(void) __compiletime_error(msg); \
> +               if (!(condition))                                       \
> +                       prefix ## suffix();                             \
> +       } while (0)
> +#define _compiletime_assert(condition, msg, prefix, suffix) \
> +       __compiletime_assert(condition, msg, prefix, suffix)
> +#define compiletime_assert(condition, msg) \
> +       _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> +#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> +#define BUILD_BUG_ON(condition) \
> +       BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
> +
> +#define ARRAY_SIZE(a)          (sizeof (a) / sizeof ((a)[0]))
> +
> +#define MODULE_LICENSE(str)    /* str */
> +
> +typedef uint8_t                        u8;
> +typedef uint16_t               u16;
> +typedef uint32_t               u32;
> +typedef uint64_t               u64;
> +
> +#define TEST_PASS      0
> +#define TEST_SKIP      1
> +#define TEST_FAIL      2
> +struct kunit {
> +       int status;
> +       char *msg;
> +};
> +struct kunit_case {
> +        void (*run_case)(struct kunit *test);
> +        const char *name;
> +};

Nit: tabs/spaces.

There are a few other checkpatch warnings, mostly it just complaining
about the use of 'return' in macros, which I think is probably still
better than trying to hack something out of setjmp/longjmp at this
point.

> +struct kunit_suite {
> +       const char *name;
> +       const struct kunit_case *test_cases;
> +};
> +#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> +
> +#define KUNIT_ASSERT_TRUE_MSG(test, expr, fmt, ...)                    \
> +do {                                                                   \
> +       if (!(expr)) {                                                  \
> +               if (test->status != TEST_SKIP)                          \
> +                       test->status = TEST_FAIL;                       \
> +               if (test->msg)                                          \
> +                       free(test->msg);                                \
> +               asprintf(&test->msg, fmt, ##__VA_ARGS__);               \
> +               return;                                                 \
> +       }                                                               \
> +} while (0)
> +
> +#define KUNIT_ASSERT_EQ_MSG(test, left, right, fmt, ...)               \
> +       KUNIT_ASSERT_TRUE_MSG(test, (left) == (right),                  \
> +                             #left " != " #right ": " fmt,             \
> +                             ##__VA_ARGS__)
> +
> +#define kunit_skip(test, fmt, ...)                                     \
> +do {                                                                   \
> +       test->status = TEST_SKIP;                                       \
> +       if (test->msg)                                                  \
> +               free(test->msg);                                        \
> +       asprintf(&test->msg, fmt, ##__VA_ARGS__);                       \
> +       return;                                                         \
> +} while (0)
> +
> +static int do_kunit_test_suite(struct kunit_suite *suite)
> +{
> +       const struct kunit_case *test_case;
> +       int pass = 0, fail = 0, skip = 0;
> +       int rc = 0;
> +       size_t i = 0;
> +
> +       printf("  TAP version 14\n");
> +       for (test_case = suite->test_cases; test_case->run_case; test_case++)
> +               i++;
> +       printf("  1..%zu\n", i);
> +       i = 0;
> +       for (test_case = suite->test_cases; test_case->run_case; test_case++) {
> +               struct kunit test = { };
> +
> +               i++;
> +               test_case->run_case(&test);
> +               switch (test.status) {
> +               default:
> +               case TEST_FAIL:
> +                       fprintf(stderr, "  not ok %zu - %s%s%s",
> +                               i, test_case->name,
> +                               test.msg ? " # ERROR " : "",
> +                               test.msg ?: "\n");
> +                       rc = 1;

What is this function trying to return? If 'rc' is supposed to be a
result status, this should be TEST_FAIL (2)?

As-is, when a test case fails, the whole suite is being marked as SKIPPED.

The other thing worth noting is that -- if this is fixed -- there's no
way a whole suite can be marked SKIPPED. KUnit will mark a suite as
skipped if all of its subtests are skipped. (This is a much more niche
case, though.)

> +                       fail++;
> +                       break;
> +               case TEST_SKIP:
> +                       fprintf(stdout, "  ok %zu - %s # SKIP%s%s",
> +                               i, test_case->name,
> +                               test.msg ? " " : "",
> +                               test.msg ?: "\n");
> +                       skip++;
> +                       break;
> +               case TEST_PASS:
> +                       fprintf(stdout, "  ok %zu - %s\n",
> +                               i, test_case->name);
> +                       pass++;
> +                       break;
> +               }
> +               if (test.msg)
> +                       free(test.msg);
> +       }
> +       printf("# %s: pass:%d fail:%d skip:%d total:%zu\n",
> +               suite->name, pass, fail, skip, i);
> +       return rc;
> +}
> +
> +static int run_suites(char *name, struct kunit_suite *suites[], size_t count)
> +{
> +       int pass = 0, fail = 0, skip = 0;
> +       int one, ret = 0;
> +       size_t i;
> +
> +       printf("TAP version 14\n");
> +       printf("1..%zu\n", count);
> +       for (i = 0; i < count; ++i) {
> +               one = do_kunit_test_suite(suites[i]);
> +               switch (one) {
> +               case TEST_SKIP:
> +                       skip++;
> +                       break;
> +               case TEST_PASS:
> +                       pass++;
> +                       break;
> +               default:
> +                       fail++;
> +                       break;
> +               }
> +               printf("%sok %zu - %s\n",
> +                       one == TEST_FAIL ? "not " : "",
> +                       i + 1, suites[i]->name);
> +               ret |= one;
> +       }
> +       printf("# %s: pass:%d fail:%d skip:%d total:%zu\n",
> +               name, pass, fail, skip, count);
> +       return ret;
> +}
> +
> +#define kunit_test_suites(suite...)                            \
> +int main(int argc, char *argv[]) {                             \

Nit: checkpatch really wants the '{' on the next line.

> +       static struct kunit_suite *suites[] = { suite };        \
> +       return run_suites(argv[0], suites, ARRAY_SIZE(suites)); \
> +}
> +
> +#endif /* __UAPI_MISC_KUNIT_H__ */
> diff --git a/lib/stackinit_kunit.c b/lib/stackinit_kunit.c
> index 35c69aa425b2..6d468630c90a 100644
> --- a/lib/stackinit_kunit.c
> +++ b/lib/stackinit_kunit.c
> @@ -8,7 +8,13 @@
>   *             --make_option LLVM=1 \
>   *             --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y
>   *
> + * External build example:
> + *     clang -O2 -Wall -ftrivial-auto-var-init=pattern \
> + *             -o stackinit_kunit stackinit_kunit.c
> + *     ./stackinit_kunit
> + *
>   */
> +#ifdef __KERNEL__
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>  #include <kunit/test.h>
> @@ -17,6 +23,11 @@
>  #include <linux/module.h>
>  #include <linux/string.h>
>
> +#else
> +/* Userspace KUnit stub header. */
> +#include <misc/kunit.h>
> +#endif
> +
>  /* Exfiltration buffer. */
>  #define MAX_VAR_SIZE   128
>  static u8 check_buf[MAX_VAR_SIZE];
> --
> 2.32.0
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature