| Message ID | 20220709032001.819487-2-davidgow@google.com |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Rework KUnit test execution in modules | expand |
Hi David, Jeremy, On Sat, Jul 9, 2022 at 5:21 AM David Gow <davidgow@google.com> wrote: > From: Jeremy Kerr <jk@codeconstruct.com.au> > > Currently, KUnit runs built-in tests and tests loaded from modules > differently. For built-in tests, the kunit_test_suite{,s}() macro adds a > list of suites in the .kunit_test_suites linker section. However, for > kernel modules, a module_init() function is used to run the test suites. > > This causes problems if tests are included in a module which already > defines module_init/exit_module functions, as they'll conflict with the > kunit-provided ones. > > This change removes the kunit-defined module inits, and instead parses > the kunit tests from their own section in the module. After module init, > we call __kunit_test_suites_init() on the contents of that section, > which prepares and runs the suite. > > This essentially unifies the module- and non-module kunit init formats. > > Tested-by: Maíra Canal <maira.canal@usp.br> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > Signed-off-by: Daniel Latypov <dlatypov@google.com> > Signed-off-by: David Gow <davidgow@google.com> Thanks for your patch, which is now commit 3d6e44623841c8b8 ("kunit: unify module and builtin suite definitions") upstream. Since this commit, modular kunit tests are no longer run at all. Before: # insmod lib/kunit/kunit.ko # insmod lib/test_hash.ko test_hash: loading test module taints kernel. # Subtest: hash 1..2 ok 1 - test_string_or ok 2 - test_hash_or # hash: pass:2 fail:0 skip:0 total:2 # Totals: pass:2 fail:0 skip:0 total:2 ok 1 - hash After: # insmod lib/kunit/kunit.ko # insmod lib/test_hash.ko test_hash: loading test module taints kernel. The actual test code (and test init code, if it exists) is not run. Reverting commits e5857d396f35e59e ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites") and 3d6e44623841c8b8 ("kunit: unify module and builtin suite definitions") fixes the issue. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Aug 11, 2022 at 9:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi David, Jeremy, > > On Sat, Jul 9, 2022 at 5:21 AM David Gow <davidgow@google.com> wrote: > > From: Jeremy Kerr <jk@codeconstruct.com.au> > > > > Currently, KUnit runs built-in tests and tests loaded from modules > > differently. For built-in tests, the kunit_test_suite{,s}() macro adds a > > list of suites in the .kunit_test_suites linker section. However, for > > kernel modules, a module_init() function is used to run the test suites. > > > > This causes problems if tests are included in a module which already > > defines module_init/exit_module functions, as they'll conflict with the > > kunit-provided ones. > > > > This change removes the kunit-defined module inits, and instead parses > > the kunit tests from their own section in the module. After module init, > > we call __kunit_test_suites_init() on the contents of that section, > > which prepares and runs the suite. > > > > This essentially unifies the module- and non-module kunit init formats. > > > > Tested-by: Maíra Canal <maira.canal@usp.br> > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > Signed-off-by: David Gow <davidgow@google.com> > > Thanks for your patch, which is now commit 3d6e44623841c8b8 ("kunit: > unify module and builtin suite definitions") upstream. > > Since this commit, modular kunit tests are no longer run at all. > > Before: > > # insmod lib/kunit/kunit.ko > # insmod lib/test_hash.ko > test_hash: loading test module taints kernel. > # Subtest: hash > 1..2 > ok 1 - test_string_or > ok 2 - test_hash_or > # hash: pass:2 fail:0 skip:0 total:2 > # Totals: pass:2 fail:0 skip:0 total:2 > ok 1 - hash > > After: > > # insmod lib/kunit/kunit.ko > # insmod lib/test_hash.ko > test_hash: loading test module taints kernel. > > The actual test code (and test init code, if it exists) is not run. > > Reverting commits e5857d396f35e59e ("kunit: flatten kunit_suite*** > to kunit_suite** in .kunit_test_suites") and 3d6e44623841c8b8 ("kunit: > unify module and builtin suite definitions") fixes the issue. Thanks Geert, This is a known issue. There's a patch to fix it here, which just missed the pull request: https://patchwork.kernel.org/project/linux-kselftest/patch/20220713005221.1926290-1-davidgow@google.com/ We'll try to get it merged as soon as possible. Cheers, -- David
Hi David, On Thu, Aug 11, 2022 at 6:56 PM David Gow <davidgow@google.com> wrote: > On Thu, Aug 11, 2022 at 9:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Sat, Jul 9, 2022 at 5:21 AM David Gow <davidgow@google.com> wrote: > > > From: Jeremy Kerr <jk@codeconstruct.com.au> > > > > > > Currently, KUnit runs built-in tests and tests loaded from modules > > > differently. For built-in tests, the kunit_test_suite{,s}() macro adds a > > > list of suites in the .kunit_test_suites linker section. However, for > > > kernel modules, a module_init() function is used to run the test suites. > > > > > > This causes problems if tests are included in a module which already > > > defines module_init/exit_module functions, as they'll conflict with the > > > kunit-provided ones. > > > > > > This change removes the kunit-defined module inits, and instead parses > > > the kunit tests from their own section in the module. After module init, > > > we call __kunit_test_suites_init() on the contents of that section, > > > which prepares and runs the suite. > > > > > > This essentially unifies the module- and non-module kunit init formats. > > > > > > Tested-by: Maíra Canal <maira.canal@usp.br> > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > Signed-off-by: David Gow <davidgow@google.com> > > > > Thanks for your patch, which is now commit 3d6e44623841c8b8 ("kunit: > > unify module and builtin suite definitions") upstream. > > > > Since this commit, modular kunit tests are no longer run at all. > > > > Before: > > > > # insmod lib/kunit/kunit.ko > > # insmod lib/test_hash.ko > > test_hash: loading test module taints kernel. > > # Subtest: hash > > 1..2 > > ok 1 - test_string_or > > ok 2 - test_hash_or > > # hash: pass:2 fail:0 skip:0 total:2 > > # Totals: pass:2 fail:0 skip:0 total:2 > > ok 1 - hash > > > > After: > > > > # insmod lib/kunit/kunit.ko > > # insmod lib/test_hash.ko > > test_hash: loading test module taints kernel. > > > > The actual test code (and test init code, if it exists) is not run. > > > > Reverting commits e5857d396f35e59e ("kunit: flatten kunit_suite*** > > to kunit_suite** in .kunit_test_suites") and 3d6e44623841c8b8 ("kunit: > > unify module and builtin suite definitions") fixes the issue. > > Thanks Geert, > > This is a known issue. There's a patch to fix it here, which just > missed the pull request: > https://patchwork.kernel.org/project/linux-kselftest/patch/20220713005221.1926290-1-davidgow@google.com/ > > We'll try to get it merged as soon as possible. Thanks for the pointer. I can confirm it fixes the issue, so I provided my Tb tag. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/include/kunit/test.h b/include/kunit/test.h index 7646d1bcf685..cb155d3da284 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -250,42 +250,9 @@ static inline int kunit_run_all_tests(void) } #endif /* IS_BUILTIN(CONFIG_KUNIT) */ -#ifdef MODULE -/** - * kunit_test_suites_for_module() - used to register one or more - * &struct kunit_suite with KUnit. - * - * @__suites: a statically allocated list of &struct kunit_suite. - * - * Registers @__suites with the test framework. See &struct kunit_suite for - * more information. - * - * If a test suite is built-in, module_init() gets translated into - * an initcall which we don't want as the idea is that for builtins - * the executor will manage execution. So ensure we do not define - * module_{init|exit} functions for the builtin case when registering - * suites via kunit_test_suites() below. - */ -#define kunit_test_suites_for_module(__suites) \ - static int __init kunit_test_suites_init(void) \ - { \ - return __kunit_test_suites_init(__suites); \ - } \ - module_init(kunit_test_suites_init); \ - \ - static void __exit kunit_test_suites_exit(void) \ - { \ - return __kunit_test_suites_exit(__suites); \ - } \ - module_exit(kunit_test_suites_exit) \ - MODULE_INFO(test, "Y"); -#else -#define kunit_test_suites_for_module(__suites) -#endif /* MODULE */ - #define __kunit_test_suites(unique_array, unique_suites, ...) \ + MODULE_INFO(test, "Y"); \ static struct kunit_suite *unique_array[] = { __VA_ARGS__, NULL }; \ - kunit_test_suites_for_module(unique_array); \ static struct kunit_suite **unique_suites \ __used __section(".kunit_test_suites") = unique_array @@ -295,16 +262,12 @@ static inline int kunit_run_all_tests(void) * * @__suites: a statically allocated list of &struct kunit_suite. * - * Registers @suites with the test framework. See &struct kunit_suite for - * more information. - * - * When builtin, KUnit tests are all run via executor; this is done - * by placing the array of struct kunit_suite * in the .kunit_test_suites - * ELF section. + * Registers @suites with the test framework. + * This is done by placing the array of struct kunit_suite * in the + * .kunit_test_suites ELF section. * - * An alternative is to build the tests as a module. Because modules do not - * support multiple initcall()s, we need to initialize an array of suites for a - * module. + * When builtin, KUnit tests are all run via the executor at boot, and when + * built as a module, they run on module load. * */ #define kunit_test_suites(__suites...) \ diff --git a/include/linux/module.h b/include/linux/module.h index abd9fa916b7d..2490223c975d 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -505,6 +505,11 @@ struct module { int num_static_call_sites; struct static_call_site *static_call_sites; #endif +#if IS_ENABLED(CONFIG_KUNIT) + int num_kunit_suites; + struct kunit_suite ***kunit_suites; +#endif + #ifdef CONFIG_LIVEPATCH bool klp; /* Is this a livepatch module? */ diff --git a/kernel/module/main.c b/kernel/module/main.c index fed58d30725d..4542db7cdf54 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2087,6 +2087,12 @@ static int find_module_sections(struct module *mod, struct load_info *info) sizeof(*mod->static_call_sites), &mod->num_static_call_sites); #endif +#ifdef CONFIG_KUNIT + mod->kunit_suites = section_objs(info, ".kunit_test_suites", + sizeof(*mod->kunit_suites), + &mod->num_kunit_suites); +#endif + mod->extable = section_objs(info, "__ex_table", sizeof(*mod->extable), &mod->num_exentries); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 8b11552dc215..246645eb3cef 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -10,6 +10,7 @@ #include <kunit/test.h> #include <kunit/test-bug.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/moduleparam.h> #include <linux/panic.h> #include <linux/sched/debug.h> @@ -613,6 +614,49 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit); +#ifdef CONFIG_MODULES +static void kunit_module_init(struct module *mod) +{ + unsigned int i; + + for (i = 0; i < mod->num_kunit_suites; i++) + __kunit_test_suites_init(mod->kunit_suites[i]); +} + +static void kunit_module_exit(struct module *mod) +{ + unsigned int i; + + for (i = 0; i < mod->num_kunit_suites; i++) + __kunit_test_suites_exit(mod->kunit_suites[i]); +} + +static int kunit_module_notify(struct notifier_block *nb, unsigned long val, + void *data) +{ + struct module *mod = data; + + switch (val) { + case MODULE_STATE_LIVE: + kunit_module_init(mod); + break; + case MODULE_STATE_GOING: + kunit_module_exit(mod); + break; + case MODULE_STATE_COMING: + case MODULE_STATE_UNFORMED: + break; + } + + return 0; +} + +static struct notifier_block kunit_mod_nb = { + .notifier_call = kunit_module_notify, + .priority = 0, +}; +#endif + struct kunit_kmalloc_array_params { size_t n; size_t size; @@ -707,13 +751,19 @@ EXPORT_SYMBOL_GPL(kunit_cleanup); static int __init kunit_init(void) { kunit_debugfs_init(); - +#ifdef CONFIG_MODULES + return register_module_notifier(&kunit_mod_nb); +#else return 0; +#endif } late_initcall(kunit_init); static void __exit kunit_exit(void) { +#ifdef CONFIG_MODULES + unregister_module_notifier(&kunit_mod_nb); +#endif kunit_debugfs_cleanup(); } module_exit(kunit_exit);