Message-ID: <CABVgOSnbBzjcb_zt=YJ8p8Rm97s2ZYp=YvjThB_NCZD9BJQaSg@mail.gmail.com>
Date: Mon, 8 Jan 2024 15:27:37 +0800
From: David Gow <davidgow@...gle.com>
To: Marco Pagani <marpagan@...hat.com>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>, Shuah Khan <skhan@...uxfoundation.org>, 
	Jinjie Ruan <ruanjinjie@...wei.com>, Rae Moar <rmoar@...gle.com>, 
	Richard Fitzgerald <rf@...nsource.cirrus.com>, Javier Martinez Canillas <javierm@...hat.com>, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kunit: run test suites only after module
 initialization completes

On Wed, 6 Dec 2023 at 23:07, Marco Pagani <marpagan@...hat.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.
>
> 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()")
> Tested-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>
> Signed-off-by: Marco Pagani <marpagan@...hat.com>
> ---

Sorry for the delay here: there are enough subtleties here that I
wanted to double check some things.

I keep feeling that there has to be a nicer way of doing this, but I
can't think of one, so let's go with this, since it's fixing a real
issue.

I'm a little hesitant about our use of the suite_set.start address as
an 'is initialised' flag, and depending on it being reallocated via
kunit_filter_suites(), but since we already depend on that (by always
using kunit_free_suite_set()), I'm okay with it.

My only request (other than this needing a rebase, probably on top of
6.8) would be to add a comment in kunit_filter_suites() noting that it
must return a virtual address. That's probably something we should've
done a while ago, but I can just see this requirement getting
forgotten.

Reviewed-by: David Gow <davidgow@...gle.com>

Cheers,
-- David

>  lib/kunit/test.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 7aceb07a1af9..3263e0d5e0f6 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"
> @@ -775,12 +776,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,
> @@ -790,12 +798,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: 33cc938e65a98f1d29d0a18403dbbee050dcad9a
> --
> 2.43.0
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)

Powered by blists - more mailing lists