[prev in list] [next in list] [prev in thread] [next in thread]
List: kvm
Subject: Re: [PATCH v1 1/8] kunit: Run tests when the kernel is fully setup
From: Kees Cook <keescook () chromium ! org>
Date: 2024-02-29 18:21:19
Message-ID: 202402291019.17D24E9 () keescook
[Download RAW message or body]
On Thu, Feb 29, 2024 at 06:04:02PM +0100, Mickaël Salaün wrote:
> Run all the KUnit tests just before the first userspace code is
> launched. This makes it it possible to write new tests that check the
> kernel in its final state i.e., with all async __init code called,
> memory and RCU properly set up, and sysctl boot arguments evaluated.
>
> The initial motivation is to run hardening tests (e.g. memory
> protection, Heki's CR-pinning), which require such security protection
> to be fully setup (e.g. memory marked as read-only).
>
> Because the suite set could refer to init data, initialize the suite set
> with late_initcall(), before kunit_run_all_tests(), if KUnit is built-in
> and enabled at boot time. To make it more consistent and easier to
> manage, whatever filters are used or not, always copy test suite entries
> and free them after all tests are run.
>
> Because of the prepare_namespace() call, we need to have a valid root
> filesystem. To make it simple, let's use tmpfs with an empty root.
> Teach kunit_kernel.py:LinuxSourceTreeOperations*() about the related
> kernel boot argument, and add this filesystem to the kunit.py's kernel
> build requirements.
>
> Remove __init and __refdata markers from iov_iter, bitfield, checksum,
> and the example KUnit tests. Without this change, the kernel tries to
> execute NX-protected pages (because the pages are deallocated).
I defer to the KUnit maintainers, but I'd find this patch easier to
review if the __init and __refdata changes were a separate patch.
But otherwise, I like the goals of this series! It should make adding a
userspace memory region for porting the usercopy tests easier too.
-Kees
>
> Tested with:
> ./tools/testing/kunit/kunit.py run --alltests
> ./tools/testing/kunit/kunit.py run --alltests --arch x86_64
>
> Cc: Alan Maguire <alan.maguire@oracle.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: David Gow <davidgow@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Marco Pagani <marpagan@redhat.com>
> Cc: Rae Moar <rmoar@google.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> init/main.c | 4 +-
> lib/bitfield_kunit.c | 8 +--
> lib/checksum_kunit.c | 2 +-
> lib/kunit/executor.c | 81 +++++++++++++++++++++--------
> lib/kunit/kunit-example-test.c | 6 +--
> lib/kunit_iov_iter.c | 52 +++++++++---------
> tools/testing/kunit/kunit_kernel.py | 6 ++-
> 7 files changed, 96 insertions(+), 63 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index e24b0780fdff..b39d74727aad 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1463,6 +1463,8 @@ static int __ref kernel_init(void *unused)
>
> do_sysctl_args();
>
> + kunit_run_all_tests();
> +
> if (ramdisk_execute_command) {
> ret = run_init_process(ramdisk_execute_command);
> if (!ret)
> @@ -1550,8 +1552,6 @@ static noinline void __init kernel_init_freeable(void)
>
> do_basic_setup();
>
> - kunit_run_all_tests();
> -
> wait_for_initramfs();
> console_on_rootfs();
>
> diff --git a/lib/bitfield_kunit.c b/lib/bitfield_kunit.c
> index 1473d8b4bf0f..71e9f2e96496 100644
> --- a/lib/bitfield_kunit.c
> +++ b/lib/bitfield_kunit.c
> @@ -57,7 +57,7 @@
> CHECK_ENC_GET_BE(tp, v, field, res); \
> } while (0)
>
> -static void __init test_bitfields_constants(struct kunit *context)
> +static void test_bitfields_constants(struct kunit *context)
> {
> /*
> * NOTE
> @@ -100,7 +100,7 @@ static void __init test_bitfields_constants(struct kunit \
> *context) tp##_encode_bits(v, mask) != v << __ffs64(mask));\
> } while (0)
>
> -static void __init test_bitfields_variables(struct kunit *context)
> +static void test_bitfields_variables(struct kunit *context)
> {
> CHECK(u8, 0x0f);
> CHECK(u8, 0xf0);
> @@ -126,7 +126,7 @@ static void __init test_bitfields_variables(struct kunit \
> *context) }
>
> #ifdef TEST_BITFIELD_COMPILE
> -static void __init test_bitfields_compile(struct kunit *context)
> +static void test_bitfields_compile(struct kunit *context)
> {
> /* these should fail compilation */
> CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
> @@ -137,7 +137,7 @@ static void __init test_bitfields_compile(struct kunit \
> *context) }
> #endif
>
> -static struct kunit_case __refdata bitfields_test_cases[] = {
> +static struct kunit_case bitfields_test_cases[] = {
> KUNIT_CASE(test_bitfields_constants),
> KUNIT_CASE(test_bitfields_variables),
> {}
> diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c
> index 225bb7701460..41aaed3a4963 100644
> --- a/lib/checksum_kunit.c
> +++ b/lib/checksum_kunit.c
> @@ -620,7 +620,7 @@ static void test_csum_ipv6_magic(struct kunit *test)
> #endif /* !CONFIG_NET */
> }
>
> -static struct kunit_case __refdata checksum_test_cases[] = {
> +static struct kunit_case checksum_test_cases[] = {
> KUNIT_CASE(test_csum_fixed_random_inputs),
> KUNIT_CASE(test_csum_all_carry_inputs),
> KUNIT_CASE(test_csum_no_carry_inputs),
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 689fff2b2b10..ff3e66ffa739 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -15,6 +15,8 @@ extern struct kunit_suite * const __kunit_suites_end[];
> extern struct kunit_suite * const __kunit_init_suites_start[];
> extern struct kunit_suite * const __kunit_init_suites_end[];
>
> +static struct kunit_suite_set final_suite_set = {};
> +
> static char *action_param;
>
> module_param_named(action, action_param, charp, 0400);
> @@ -233,6 +235,21 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
> if (!filtered_suite)
> continue;
>
> + if (filtered_suite == suite_set->start[i]) {
> + /*
> + * To make memory allocation consistent whatever
> + * filters are used or not, and to keep
> + * kunit_free_suite_set() simple, always copy static
> + * data.
> + */
> + filtered_suite = kmemdup(filtered_suite, sizeof(*filtered_suite),
> + GFP_KERNEL);
> + if (!filtered_suite) {
> + *err = -ENOMEM;
> + goto free_parsed_filters;
> + }
> + }
> +
> *copy++ = filtered_suite;
> }
> filtered.start = copy_start;
> @@ -348,7 +365,7 @@ static void kunit_handle_shutdown(void)
>
> }
>
> -int kunit_run_all_tests(void)
> +static int kunit_init_suites(void)
> {
> struct kunit_suite_set suite_set = {NULL, NULL};
> struct kunit_suite_set filtered_suite_set = {NULL, NULL};
> @@ -361,6 +378,9 @@ int kunit_run_all_tests(void)
> size_t init_num_suites = init_suite_set.end - init_suite_set.start;
> int err = 0;
>
> + if (!kunit_enabled())
> + return 0;
> +
> if (init_num_suites > 0) {
> suite_set = kunit_merge_suite_sets(init_suite_set, normal_suite_set);
> if (!suite_set.start)
> @@ -368,41 +388,56 @@ int kunit_run_all_tests(void)
> } else
> suite_set = normal_suite_set;
>
> - if (!kunit_enabled()) {
> - pr_info("kunit: disabled\n");
> + filtered_suite_set = kunit_filter_suites(&suite_set, filter_glob_param,
> + filter_param, filter_action_param, &err);
> +
> + /* Free original suite set before using filtered suite set */
> + if (init_num_suites > 0)
> + kfree(suite_set.start);
> + suite_set = filtered_suite_set;
> +
> + if (err) {
> + pr_err("kunit executor: error filtering suites: %d\n", err);
> goto free_out;
> }
>
> - if (filter_glob_param || filter_param) {
> - filtered_suite_set = kunit_filter_suites(&suite_set, filter_glob_param,
> - filter_param, filter_action_param, &err);
> + final_suite_set = suite_set;
> + return 0;
>
> - /* Free original suite set before using filtered suite set */
> - if (init_num_suites > 0)
> - kfree(suite_set.start);
> - suite_set = filtered_suite_set;
> +free_out:
> + kunit_free_suite_set(suite_set);
>
> - if (err) {
> - pr_err("kunit executor: error filtering suites: %d\n", err);
> - goto free_out;
> - }
> +out:
> + kunit_handle_shutdown();
> + return err;
> +}
> +
> +late_initcall(kunit_init_suites);
> +
> +int kunit_run_all_tests(void)
> +{
> + int err = 0;
> +
> + if (!kunit_enabled()) {
> + pr_info("kunit: disabled\n");
> + goto out;
> }
>
> + if (!final_suite_set.start)
> + goto out;
> +
> if (!action_param)
> - kunit_exec_run_tests(&suite_set, true);
> + kunit_exec_run_tests(&final_suite_set, true);
> else if (strcmp(action_param, "list") == 0)
> - kunit_exec_list_tests(&suite_set, false);
> + kunit_exec_list_tests(&final_suite_set, false);
> else if (strcmp(action_param, "list_attr") == 0)
> - kunit_exec_list_tests(&suite_set, true);
> + kunit_exec_list_tests(&final_suite_set, true);
> else
> pr_err("kunit executor: unknown action '%s'\n", action_param);
>
> -free_out:
> - if (filter_glob_param || filter_param)
> - kunit_free_suite_set(suite_set);
> - else if (init_num_suites > 0)
> - /* Don't use kunit_free_suite_set because suites aren't individually allocated \
> */
> - kfree(suite_set.start);
> + kunit_free_suite_set(final_suite_set);
> + final_suite_set.start = NULL;
> + final_suite_set.end = NULL;
>
> out:
> kunit_handle_shutdown();
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index 798924f7cc86..248949eb3b16 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -337,7 +337,7 @@ static struct kunit_suite example_test_suite = {
> */
> kunit_test_suites(&example_test_suite);
>
> -static int __init init_add(int x, int y)
> +static int init_add(int x, int y)
> {
> return (x + y);
> }
> @@ -345,7 +345,7 @@ static int __init init_add(int x, int y)
> /*
> * This test should always pass. Can be used to test init suites.
> */
> -static void __init example_init_test(struct kunit *test)
> +static void example_init_test(struct kunit *test)
> {
> KUNIT_EXPECT_EQ(test, init_add(1, 1), 2);
> }
> @@ -354,7 +354,7 @@ static void __init example_init_test(struct kunit *test)
> * The kunit_case struct cannot be marked as __initdata as this will be
> * used in debugfs to retrieve results after test has run
> */
> -static struct kunit_case __refdata example_init_test_cases[] = {
> +static struct kunit_case example_init_test_cases[] = {
> KUNIT_CASE(example_init_test),
> {}
> };
> diff --git a/lib/kunit_iov_iter.c b/lib/kunit_iov_iter.c
> index 859b67c4d697..a77991a9bffb 100644
> --- a/lib/kunit_iov_iter.c
> +++ b/lib/kunit_iov_iter.c
> @@ -44,9 +44,8 @@ static void iov_kunit_unmap(void *data)
> vunmap(data);
> }
>
> -static void *__init iov_kunit_create_buffer(struct kunit *test,
> - struct page ***ppages,
> - size_t npages)
> +static void *iov_kunit_create_buffer(struct kunit *test, struct page ***ppages,
> + size_t npages)
> {
> struct page **pages;
> unsigned long got;
> @@ -69,11 +68,10 @@ static void *__init iov_kunit_create_buffer(struct kunit *test,
> return buffer;
> }
>
> -static void __init iov_kunit_load_kvec(struct kunit *test,
> - struct iov_iter *iter, int dir,
> - struct kvec *kvec, unsigned int kvmax,
> - void *buffer, size_t bufsize,
> - const struct kvec_test_range *pr)
> +static void iov_kunit_load_kvec(struct kunit *test, struct iov_iter *iter,
> + int dir, struct kvec *kvec, unsigned int kvmax,
> + void *buffer, size_t bufsize,
> + const struct kvec_test_range *pr)
> {
> size_t size = 0;
> int i;
> @@ -95,7 +93,7 @@ static void __init iov_kunit_load_kvec(struct kunit *test,
> /*
> * Test copying to a ITER_KVEC-type iterator.
> */
> -static void __init iov_kunit_copy_to_kvec(struct kunit *test)
> +static void iov_kunit_copy_to_kvec(struct kunit *test)
> {
> const struct kvec_test_range *pr;
> struct iov_iter iter;
> @@ -145,7 +143,7 @@ static void __init iov_kunit_copy_to_kvec(struct kunit *test)
> /*
> * Test copying from a ITER_KVEC-type iterator.
> */
> -static void __init iov_kunit_copy_from_kvec(struct kunit *test)
> +static void iov_kunit_copy_from_kvec(struct kunit *test)
> {
> const struct kvec_test_range *pr;
> struct iov_iter iter;
> @@ -213,12 +211,11 @@ static const struct bvec_test_range bvec_test_ranges[] = {
> { -1, -1, -1 }
> };
>
> -static void __init iov_kunit_load_bvec(struct kunit *test,
> - struct iov_iter *iter, int dir,
> - struct bio_vec *bvec, unsigned int bvmax,
> - struct page **pages, size_t npages,
> - size_t bufsize,
> - const struct bvec_test_range *pr)
> +static void iov_kunit_load_bvec(struct kunit *test, struct iov_iter *iter,
> + int dir, struct bio_vec *bvec,
> + unsigned int bvmax, struct page **pages,
> + size_t npages, size_t bufsize,
> + const struct bvec_test_range *pr)
> {
> struct page *can_merge = NULL, *page;
> size_t size = 0;
> @@ -254,7 +251,7 @@ static void __init iov_kunit_load_bvec(struct kunit *test,
> /*
> * Test copying to a ITER_BVEC-type iterator.
> */
> -static void __init iov_kunit_copy_to_bvec(struct kunit *test)
> +static void iov_kunit_copy_to_bvec(struct kunit *test)
> {
> const struct bvec_test_range *pr;
> struct iov_iter iter;
> @@ -308,7 +305,7 @@ static void __init iov_kunit_copy_to_bvec(struct kunit *test)
> /*
> * Test copying from a ITER_BVEC-type iterator.
> */
> -static void __init iov_kunit_copy_from_bvec(struct kunit *test)
> +static void iov_kunit_copy_from_bvec(struct kunit *test)
> {
> const struct bvec_test_range *pr;
> struct iov_iter iter;
> @@ -370,10 +367,9 @@ static void iov_kunit_destroy_xarray(void *data)
> kfree(xarray);
> }
>
> -static void __init iov_kunit_load_xarray(struct kunit *test,
> - struct iov_iter *iter, int dir,
> - struct xarray *xarray,
> - struct page **pages, size_t npages)
> +static void iov_kunit_load_xarray(struct kunit *test, struct iov_iter *iter,
> + int dir, struct xarray *xarray,
> + struct page **pages, size_t npages)
> {
> size_t size = 0;
> int i;
> @@ -401,7 +397,7 @@ static struct xarray *iov_kunit_create_xarray(struct kunit \
> *test) /*
> * Test copying to a ITER_XARRAY-type iterator.
> */
> -static void __init iov_kunit_copy_to_xarray(struct kunit *test)
> +static void iov_kunit_copy_to_xarray(struct kunit *test)
> {
> const struct kvec_test_range *pr;
> struct iov_iter iter;
> @@ -459,7 +455,7 @@ static void __init iov_kunit_copy_to_xarray(struct kunit *test)
> /*
> * Test copying from a ITER_XARRAY-type iterator.
> */
> -static void __init iov_kunit_copy_from_xarray(struct kunit *test)
> +static void iov_kunit_copy_from_xarray(struct kunit *test)
> {
> const struct kvec_test_range *pr;
> struct iov_iter iter;
> @@ -522,7 +518,7 @@ static void __init iov_kunit_copy_from_xarray(struct kunit \
> *test) /*
> * Test the extraction of ITER_KVEC-type iterators.
> */
> -static void __init iov_kunit_extract_pages_kvec(struct kunit *test)
> +static void iov_kunit_extract_pages_kvec(struct kunit *test)
> {
> const struct kvec_test_range *pr;
> struct iov_iter iter;
> @@ -602,7 +598,7 @@ static void __init iov_kunit_extract_pages_kvec(struct kunit \
> *test) /*
> * Test the extraction of ITER_BVEC-type iterators.
> */
> -static void __init iov_kunit_extract_pages_bvec(struct kunit *test)
> +static void iov_kunit_extract_pages_bvec(struct kunit *test)
> {
> const struct bvec_test_range *pr;
> struct iov_iter iter;
> @@ -680,7 +676,7 @@ static void __init iov_kunit_extract_pages_bvec(struct kunit \
> *test) /*
> * Test the extraction of ITER_XARRAY-type iterators.
> */
> -static void __init iov_kunit_extract_pages_xarray(struct kunit *test)
> +static void iov_kunit_extract_pages_xarray(struct kunit *test)
> {
> const struct kvec_test_range *pr;
> struct iov_iter iter;
> @@ -756,7 +752,7 @@ static void __init iov_kunit_extract_pages_xarray(struct kunit \
> *test) KUNIT_SUCCEED();
> }
>
> -static struct kunit_case __refdata iov_kunit_cases[] = {
> +static struct kunit_case iov_kunit_cases[] = {
> KUNIT_CASE(iov_kunit_copy_to_kvec),
> KUNIT_CASE(iov_kunit_copy_from_kvec),
> KUNIT_CASE(iov_kunit_copy_to_bvec),
> diff --git a/tools/testing/kunit/kunit_kernel.py \
> b/tools/testing/kunit/kunit_kernel.py index 0b6488efed47..e1980ea58118 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -104,12 +104,13 @@ class \
> LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): self._kconfig = \
> qemu_arch_params.kconfig self._qemu_arch = qemu_arch_params.qemu_arch
> self._kernel_path = qemu_arch_params.kernel_path
> - self._kernel_command_line = qemu_arch_params.kernel_command_line + ' \
> kunit_shutdown=reboot' + self._kernel_command_line = \
> qemu_arch_params.kernel_command_line + ' kunit_shutdown=reboot rootfstype=tmpfs' \
> self._extra_qemu_params = qemu_arch_params.extra_qemu_params self._serial = \
> qemu_arch_params.serial
> def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> \
> kunit_config.Kconfig: kconfig = kunit_config.parse_from_string(self._kconfig)
> + kconfig.add_entry('TMPFS', 'y')
> kconfig.merge_in_entries(base_kunitconfig)
> return kconfig
>
> @@ -139,13 +140,14 @@ class \
> LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> \
> kunit_config.Kconfig: kconfig = kunit_config.parse_file(UML_KCONFIG_PATH)
> + kconfig.add_entry('TMPFS', 'y')
> kconfig.merge_in_entries(base_kunitconfig)
> return kconfig
>
> def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> """Runs the Linux UML binary. Must be named 'linux'."""
> linux_bin = os.path.join(build_dir, 'linux')
> - params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
> + params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt', \
> 'rootfstype=tmpfs']) return subprocess.Popen([linux_bin] + params,
> stdin=subprocess.PIPE,
> stdout=subprocess.PIPE,
> --
> 2.44.0
>
--
Kees Cook
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic