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.
v2: - add include <linux/mm.h>
Fixes: 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()") Signed-off-by: Marco Pagani marpagan@redhat.com --- lib/kunit/test.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f2eb71f1a66c..0e829b9f8ce5 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -16,6 +16,7 @@ #include <linux/panic.h> #include <linux/sched/debug.h> #include <linux/sched.h> +#include <linux/mm.h>
#include "debugfs.h" #include "hooks-impl.h" @@ -737,12 +738,14 @@ static void kunit_module_exit(struct module *mod) }; const char *action = kunit_action();
+ 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, @@ -752,12 +755,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;
base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
Marco Pagani marpagan@redhat.com writes:
Hello Marco,
[...]
@@ -737,12 +738,14 @@ static void kunit_module_exit(struct module *mod) }; const char *action = kunit_action();
- if (!suite_set.start || !virt_addr_valid(suite_set.start))
return;
I would add a comment here explaining why this condition is checked and what it means. Maybe something like the following ?
+ /* + * Check if the kunit test suite start address is a virtual + * address or a direct mapping address. This is used as an + * indication of whether the kunit_filter_suites() was used + * to filter the kunit test suite or not. + * + * If is not a virtual address, then this means that the + * kunit_module_init() function was not called and the kunit + * suite was not filtered. Let's just bail out in that case. + */ + if (!suite_set.start || !virt_addr_valid(suite_set.start)) + return;
The patch makes sense to me though and agree that is a better approach.
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
On 28/11/23 12:15, Javier Martinez Canillas wrote:
Marco Pagani marpagan@redhat.com writes:
Hello Marco,
[...]
@@ -737,12 +738,14 @@ static void kunit_module_exit(struct module *mod) }; const char *action = kunit_action();
- if (!suite_set.start || !virt_addr_valid(suite_set.start))
return;
I would add a comment here explaining why this condition is checked and what it means. Maybe something like the following ?
/*
* Check if the kunit test suite start address is a virtual
* address or a direct mapping address. This is used as an
* indication of whether the kunit_filter_suites() was used
* to filter the kunit test suite or not.
*
* If is not a virtual address, then this means that the
* kunit_module_init() function was not called and the kunit
* suite was not filtered. Let's just bail out in that case.
*/
if (!suite_set.start || !virt_addr_valid(suite_set.start))
return;
Good point. I'll add a comment in v3.
The patch makes sense to me though and agree that is a better approach.
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
Thanks, Marco
On 28/11/2023 10:16, Marco Pagani 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.
v2:
- add include <linux/mm.h>
Fixes: 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()") Signed-off-by: Marco Pagani marpagan@redhat.com
Tested-by: Richard Fitzgerald rf@opensource.cirrus.com
Fixed this crash: https://lore.kernel.org/all/e239b94b-462a-41e5-9a4c-cd1ffd530d75@opensource....
Also tested with sound/pci/hda/cirrus_scodec_test.c
linux-kselftest-mirror@lists.linaro.org