| Message ID | 20240110155948.90964-1-marpagan@redhat.com |
|---|---|
| State | Accepted |
| Commit | a1af6a2bfa0cb46d70b7df5352993e750da6c79b |
| Headers | show |
| Series | [v4] kunit: run test suites only after module initialization completes | expand |
Hi Marco, On Wed, Jan 10, 2024 at 5:01 PM Marco Pagani <marpagan@redhat.com> wrote: > Commit 2810c1e99867 ("kunit: Fix wild-memory-access bug in > kunit_free_suite_set()") fixed a wild-memory-access bug that could have > happened during the loading phase of test suites built and executed as > loadable modules. However, it also introduced a problematic side effect > that causes test suites modules to crash when they attempt to register > fake devices. > > When a module is loaded, it traverses the MODULE_STATE_UNFORMED and > MODULE_STATE_COMING states before reaching the normal operating state > MODULE_STATE_LIVE. Finally, when the module is removed, it moves to > MODULE_STATE_GOING before being released. However, if the loading > function load_module() fails between complete_formation() and > do_init_module(), the module goes directly from MODULE_STATE_COMING to > MODULE_STATE_GOING without passing through MODULE_STATE_LIVE. > > This behavior was causing kunit_module_exit() to be called without > having first executed kunit_module_init(). Since kunit_module_exit() is > responsible for freeing the memory allocated by kunit_module_init() > through kunit_filter_suites(), this behavior was resulting in a > wild-memory-access bug. > > Commit 2810c1e99867 ("kunit: Fix wild-memory-access bug in > kunit_free_suite_set()") fixed this issue by running the tests when the > module is still in MODULE_STATE_COMING. However, modules in that state > are not fully initialized, lacking sysfs kobjects. Therefore, if a test > module attempts to register a fake device, it will inevitably crash. > > This patch proposes a different approach to fix the original > wild-memory-access bug while restoring the normal module execution flow > by making kunit_module_exit() able to detect if kunit_module_init() has > previously initialized the tests suite set. In this way, test modules > can once again register fake devices without crashing. > > This behavior is achieved by checking whether mod->kunit_suites is a > virtual or direct mapping address. If it is a virtual address, then > kunit_module_init() has allocated the suite_set in kunit_filter_suites() > using kmalloc_array(). On the contrary, if mod->kunit_suites is still > pointing to the original address that was set when looking up the > kunit_test_suites section of the module, then the loading phase has > failed and there's no memory to be freed. > > v4: > - rebased on 6.8 > - noted that kunit_filter_suites() must return a virtual address > v3: > - add a comment to clarify why the start address is checked > v2: > - add include <linux/mm.h> > > Fixes: 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()") > Reviewed-by: David Gow <davidgow@google.com> > Tested-by: Rae Moar <rmoar@google.com> > Tested-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Signed-off-by: Marco Pagani <marpagan@redhat.com> Thank you, this fixes the crashes I was seeing when running platform_device_test, and which I bisected to the commit in the Fixes tag. Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 717b9599036b..689fff2b2b10 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -146,6 +146,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set) kfree(suite_set.start); } +/* + * Filter and reallocate test suites. Must return the filtered test suites set + * allocated at a valid virtual address or NULL in case of error. + */ struct kunit_suite_set kunit_filter_suites(const struct kunit_suite_set *suite_set, const char *filter_glob, diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f95d2093a0aa..31a5a992e646 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -17,6 +17,7 @@ #include <linux/panic.h> #include <linux/sched/debug.h> #include <linux/sched.h> +#include <linux/mm.h> #include "debugfs.h" #include "device-impl.h" @@ -801,12 +802,19 @@ static void kunit_module_exit(struct module *mod) }; const char *action = kunit_action(); + /* + * Check if the start address is a valid virtual address to detect + * if the module load sequence has failed and the suite set has not + * been initialized and filtered. + */ + if (!suite_set.start || !virt_addr_valid(suite_set.start)) + return; + if (!action) __kunit_test_suites_exit(mod->kunit_suites, mod->num_kunit_suites); - if (suite_set.start) - kunit_free_suite_set(suite_set); + kunit_free_suite_set(suite_set); } static int kunit_module_notify(struct notifier_block *nb, unsigned long val, @@ -816,12 +824,12 @@ static int kunit_module_notify(struct notifier_block *nb, unsigned long val, switch (val) { case MODULE_STATE_LIVE: + kunit_module_init(mod); break; case MODULE_STATE_GOING: kunit_module_exit(mod); break; case MODULE_STATE_COMING: - kunit_module_init(mod); break; case MODULE_STATE_UNFORMED: break;
Commit 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()") fixed a wild-memory-access bug that could have happened during the loading phase of test suites built and executed as loadable modules. However, it also introduced a problematic side effect that causes test suites modules to crash when they attempt to register fake devices. When a module is loaded, it traverses the MODULE_STATE_UNFORMED and MODULE_STATE_COMING states before reaching the normal operating state MODULE_STATE_LIVE. Finally, when the module is removed, it moves to MODULE_STATE_GOING before being released. However, if the loading function load_module() fails between complete_formation() and do_init_module(), the module goes directly from MODULE_STATE_COMING to MODULE_STATE_GOING without passing through MODULE_STATE_LIVE. This behavior was causing kunit_module_exit() to be called without having first executed kunit_module_init(). Since kunit_module_exit() is responsible for freeing the memory allocated by kunit_module_init() through kunit_filter_suites(), this behavior was resulting in a wild-memory-access bug. Commit 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()") fixed this issue by running the tests when the module is still in MODULE_STATE_COMING. However, modules in that state are not fully initialized, lacking sysfs kobjects. Therefore, if a test module attempts to register a fake device, it will inevitably crash. This patch proposes a different approach to fix the original wild-memory-access bug while restoring the normal module execution flow by making kunit_module_exit() able to detect if kunit_module_init() has previously initialized the tests suite set. In this way, test modules can once again register fake devices without crashing. This behavior is achieved by checking whether mod->kunit_suites is a virtual or direct mapping address. If it is a virtual address, then kunit_module_init() has allocated the suite_set in kunit_filter_suites() using kmalloc_array(). On the contrary, if mod->kunit_suites is still pointing to the original address that was set when looking up the .kunit_test_suites section of the module, then the loading phase has failed and there's no memory to be freed. v4: - rebased on 6.8 - noted that kunit_filter_suites() must return a virtual address v3: - add a comment to clarify why the start address is checked v2: - add include <linux/mm.h> Fixes: 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()") Reviewed-by: David Gow <davidgow@google.com> Tested-by: Rae Moar <rmoar@google.com> Tested-by: Richard Fitzgerald <rf@opensource.cirrus.com> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Signed-off-by: Marco Pagani <marpagan@redhat.com> --- lib/kunit/executor.c | 4 ++++ lib/kunit/test.c | 14 +++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) base-commit: 539e582a375dedee95a4fa9ca3f37cdb25c441ec