diff mbox series

[v2] kunit: run test suites only after module initialization completes

Message ID 20231128101627.65399-1-marpagan@redhat.com (mailing list archive)
State New
Delegated to: Brendan Higgins
Headers show
Series [v2] kunit: run test suites only after module initialization completes | expand

Commit Message

Marco Pagani Nov. 28, 2023, 10:16 a.m. UTC 
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(-)


base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab

Comments

Javier Martinez Canillas Nov. 28, 2023, 11:15 a.m. UTC | #1 
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>
Marco Pagani Nov. 28, 2023, 6:40 p.m. UTC | #2 
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
Richard Fitzgerald Dec. 6, 2023, 1:09 p.m. UTC | #3 
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.cirrus.com/

Also tested with sound/pci/hda/cirrus_scodec_test.c
diff mbox series

Patch

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;