[PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[David Gow]

The new KUnit module handling has KUnit test suites listed in a
.kunit_test_suites section of each module. This should be loaded when
the module is, but at the moment this only happens if KUnit is built-in.

Also load this when KUnit is enabled as a module: it'll not be usable
unless KUnit is loaded, but such modules are likely to depend on KUnit
anyway, so it's unlikely to ever be loaded needlessly.

Fixes: 3d6e44623841 ("kunit: unify module and builtin suite definitions")
Signed-off-by: David Gow <davidgow@google.com>
---
 kernel/module/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 324a770f789c..222a0b7263b6 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2094,7 +2094,7 @@ 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
+#if IS_ENABLED(CONFIG_KUNIT)
 	mod->kunit_suites = section_objs(info, ".kunit_test_suites",
 					      sizeof(*mod->kunit_suites),
 					      &mod->num_kunit_suites);
-- 
2.37.0.144.g8ac04bfd2-goog

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Daniel Latypov]

On Tue, Jul 12, 2022 at 5:52 PM David Gow <davidgow@google.com> wrote:
>
> The new KUnit module handling has KUnit test suites listed in a
> .kunit_test_suites section of each module. This should be loaded when
> the module is, but at the moment this only happens if KUnit is built-in.
>
> Also load this when KUnit is enabled as a module: it'll not be usable
> unless KUnit is loaded, but such modules are likely to depend on KUnit
> anyway, so it's unlikely to ever be loaded needlessly.

This seems reasonable to me.

Question: what happens in this case?
1. insmod <test-module>
2. insmod kunit
3. rmmod <test-module>

I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(),
for <test-module>, I think?
But we never called __kunit_test_suites_init().
My fear is what breaks as a result of this precondition break.

E.g. In the case that CONFIG_KUNIT_DEBUGFS is enabled, this includes a
call to kunit_debugfs_destroy_suite() with no previous call to
kunit_debugfs_create_suite().
That will include a call to debugfs_remove_recursive(suite->debugfs),
where suite->debugfs is an uninitialized pointer.

Maybe we can treat it as "undefined behavior" for now and proceed with
this patch.

In terms of long-term fixes, perhaps insmod kunit could trigger it to
1. run all built-in tests (IIUC, it doesn't right now)
2. run all the tests of currently loaded modules
3. track which modules already ran so if you rmmod + insmod kunit
again, it won't rerun tests?

Daniel

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Luis Chamberlain]

On Wed, Jul 13, 2022 at 08:24:32AM -0700, Daniel Latypov wrote:
> On Tue, Jul 12, 2022 at 5:52 PM David Gow <davidgow@google.com> wrote:
> >
> > The new KUnit module handling has KUnit test suites listed in a
> > .kunit_test_suites section of each module. This should be loaded when
> > the module is, but at the moment this only happens if KUnit is built-in.
> >
> > Also load this when KUnit is enabled as a module: it'll not be usable
> > unless KUnit is loaded, but such modules are likely to depend on KUnit
> > anyway, so it's unlikely to ever be loaded needlessly.
> 
> This seems reasonable to me.
> 
> Question: what happens in this case?
> 1. insmod <test-module>
> 2. insmod kunit
> 3. rmmod <test-module>
> 
> I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(),
> for <test-module>, I think?
> But we never called __kunit_test_suites_init().
> My fear is what breaks as a result of this precondition break.
> 
> E.g. In the case that CONFIG_KUNIT_DEBUGFS is enabled, this includes a
> call to kunit_debugfs_destroy_suite() with no previous call to
> kunit_debugfs_create_suite().
> That will include a call to debugfs_remove_recursive(suite->debugfs),
> where suite->debugfs is an uninitialized pointer.
> 
> Maybe we can treat it as "undefined behavior" for now and proceed with
> this patch.
> 
> In terms of long-term fixes, perhaps insmod kunit could trigger it to
> 1. run all built-in tests (IIUC, it doesn't right now)
> 2. run all the tests of currently loaded modules
> 3. track which modules already ran so if you rmmod + insmod kunit
> again, it won't rerun tests?

Let's please address these considerations.

  Luis

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3760 bytes --]

On Wed, Jul 20, 2022 at 1:58 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Jul 13, 2022 at 08:24:32AM -0700, Daniel Latypov wrote:
> > On Tue, Jul 12, 2022 at 5:52 PM David Gow <davidgow@google.com> wrote:
> > >
> > > The new KUnit module handling has KUnit test suites listed in a
> > > .kunit_test_suites section of each module. This should be loaded when
> > > the module is, but at the moment this only happens if KUnit is built-in.
> > >
> > > Also load this when KUnit is enabled as a module: it'll not be usable
> > > unless KUnit is loaded, but such modules are likely to depend on KUnit
> > > anyway, so it's unlikely to ever be loaded needlessly.
> >
> > This seems reasonable to me.
> >
> > Question: what happens in this case?
> > 1. insmod <test-module>
> > 2. insmod kunit
> > 3. rmmod <test-module>
> >
> > I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(),
> > for <test-module>, I think?
> > But we never called __kunit_test_suites_init().
> > My fear is what breaks as a result of this precondition break.

I don't think this should be possible: any module with KUnit tests
will depend on the 'kunit' module (or, at least, kunit symbols), so
shouldn't load without kunit already present.

If modprobe is used, kunit will automatically be loaded. If insmod is
used directly, loading the first module should error out with
something like:
[   82.393629] list_test: loading test module taints kernel.
[   82.409607] list_test: Unknown symbol kunit_binary_ptr_assert_format (err -2)
[   82.409657] list_test: Unknown symbol kunit_do_failed_assertion (err -2)
[   82.409799] list_test: Unknown symbol kunit_binary_assert_format (err -2)
[   82.409820] list_test: Unknown symbol kunit_unary_assert_format (err -2)
insmod: ERROR: could not insert module
/lib/modules/5.19.0-rc1-15284-g9ec67db0c271/kernel/lib/list-test.ko:
Unknown symbol in module

Maybe you could get into some trouble by force-removing modules at
various points, but you're in undefined behaviour generally at that
point, so I don't think there's much point going out-of-our-way to try
to support that.

> >
> > E.g. In the case that CONFIG_KUNIT_DEBUGFS is enabled, this includes a
> > call to kunit_debugfs_destroy_suite() with no previous call to
> > kunit_debugfs_create_suite().
> > That will include a call to debugfs_remove_recursive(suite->debugfs),
> > where suite->debugfs is an uninitialized pointer.
> >
> > Maybe we can treat it as "undefined behavior" for now and proceed with
> > this patch.
> >
> > In terms of long-term fixes, perhaps insmod kunit could trigger it to
> > 1. run all built-in tests (IIUC, it doesn't right now)
> > 2. run all the tests of currently loaded modules
> > 3. track which modules already ran so if you rmmod + insmod kunit
> > again, it won't rerun tests?
>
> Let's please address these considerations.
>

Again, I think the module and Kconfig dependencies should prevent
these situations from ever arising. It shouldn't be possible to have
any built-in tests if kunit is not itself built-in, so case (1) can't
occur. Equally, it shouldn't be possible to load any test modules
before the kunit module is loaded, so (2) can't occur. And rmmod kunit
will give an error if there are any test modules loaded: "rmmod:
ERROR: Module kunit is in use by: list_test", so (3) can't occur.

The only similar situation to (3) I can think of is where both the
test modules and kunit are rmmod-ed and reloaded. I think we'd want to
re-run the tests in that case, so tracking which modules already ran
would be counterproductive.

In short, I _think_ what we're doing now should be correct, and the
cases above are all prevented by module dependencies. But do let me
know if I'm missing something...

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Luis Chamberlain]

On Wed, Jul 20, 2022 at 05:26:02PM +0800, David Gow wrote:
> On Wed, Jul 20, 2022 at 1:58 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, Jul 13, 2022 at 08:24:32AM -0700, Daniel Latypov wrote:
> > > On Tue, Jul 12, 2022 at 5:52 PM David Gow <davidgow@google.com> wrote:
> > > >
> > > > The new KUnit module handling has KUnit test suites listed in a
> > > > .kunit_test_suites section of each module. This should be loaded when
> > > > the module is, but at the moment this only happens if KUnit is built-in.
> > > >
> > > > Also load this when KUnit is enabled as a module: it'll not be usable
> > > > unless KUnit is loaded, but such modules are likely to depend on KUnit
> > > > anyway, so it's unlikely to ever be loaded needlessly.
> > >
> > > This seems reasonable to me.
> > >
> > > Question: what happens in this case?
> > > 1. insmod <test-module>
> > > 2. insmod kunit
> > > 3. rmmod <test-module>
> > >
> > > I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(),
> > > for <test-module>, I think?
> > > But we never called __kunit_test_suites_init().
> > > My fear is what breaks as a result of this precondition break.
> 
> I don't think this should be possible: any module with KUnit tests
> will depend on the 'kunit' module (or, at least, kunit symbols), so
> shouldn't load without kunit already present.
> 
> If modprobe is used, kunit will automatically be loaded. If insmod is
> used directly, loading the first module should error out with
> something like:
> [   82.393629] list_test: loading test module taints kernel.
> [   82.409607] list_test: Unknown symbol kunit_binary_ptr_assert_format (err -2)
> [   82.409657] list_test: Unknown symbol kunit_do_failed_assertion (err -2)
> [   82.409799] list_test: Unknown symbol kunit_binary_assert_format (err -2)
> [   82.409820] list_test: Unknown symbol kunit_unary_assert_format (err -2)
> insmod: ERROR: could not insert module
> /lib/modules/5.19.0-rc1-15284-g9ec67db0c271/kernel/lib/list-test.ko:
> Unknown symbol in module

This can be fixed with a request_module() call. And since this is a
generic requirement, you can have the wrappers do it for you.

> Maybe you could get into some trouble by force-removing modules at
> various points, but you're in undefined behaviour generally at that
> point, so I don't think there's much point going out-of-our-way to try
> to support that.

You can prevent that by refcounting the kunit module / symbols, by each test.

  Luis

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[David Gow]

On Thu, Jul 21, 2022 at 4:29 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Jul 20, 2022 at 05:26:02PM +0800, David Gow wrote:
> > On Wed, Jul 20, 2022 at 1:58 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 08:24:32AM -0700, Daniel Latypov wrote:
> > > > On Tue, Jul 12, 2022 at 5:52 PM David Gow <davidgow@google.com> wrote:
> > > > >
> > > > > The new KUnit module handling has KUnit test suites listed in a
> > > > > .kunit_test_suites section of each module. This should be loaded when
> > > > > the module is, but at the moment this only happens if KUnit is built-in.
> > > > >
> > > > > Also load this when KUnit is enabled as a module: it'll not be usable
> > > > > unless KUnit is loaded, but such modules are likely to depend on KUnit
> > > > > anyway, so it's unlikely to ever be loaded needlessly.
> > > >
> > > > This seems reasonable to me.
> > > >
> > > > Question: what happens in this case?
> > > > 1. insmod <test-module>
> > > > 2. insmod kunit
> > > > 3. rmmod <test-module>
> > > >
> > > > I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(),
> > > > for <test-module>, I think?
> > > > But we never called __kunit_test_suites_init().
> > > > My fear is what breaks as a result of this precondition break.
> >
> > I don't think this should be possible: any module with KUnit tests
> > will depend on the 'kunit' module (or, at least, kunit symbols), so
> > shouldn't load without kunit already present.
> >
> > If modprobe is used, kunit will automatically be loaded. If insmod is
> > used directly, loading the first module should error out with
> > something like:
> > [   82.393629] list_test: loading test module taints kernel.
> > [   82.409607] list_test: Unknown symbol kunit_binary_ptr_assert_format (err -2)
> > [   82.409657] list_test: Unknown symbol kunit_do_failed_assertion (err -2)
> > [   82.409799] list_test: Unknown symbol kunit_binary_assert_format (err -2)
> > [   82.409820] list_test: Unknown symbol kunit_unary_assert_format (err -2)
> > insmod: ERROR: could not insert module
> > /lib/modules/5.19.0-rc1-15284-g9ec67db0c271/kernel/lib/list-test.ko:
> > Unknown symbol in module
>
> This can be fixed with a request_module() call. And since this is a
> generic requirement, you can have the wrappers do it for you.
>

I'm not convinced that this is worth the trouble, particularly since
KUnit needs to be loaded already before any test-specific code in a
module is run. _Maybe_ we could put it in the code which looks for the
.kunit_test_suites section, but even then it seems like a bit of an
ugly hack.

Personally, I'm not particularly concerned about test modules failing
to load if KUnit isn't already present -- if people want all of a
module's dependencies loaded, that's what modprobe is for.

That being said, if you feel particularly strongly about it, this is
something we can look at. Let's do so in a separate patch though: this
one does fix a regression as-is.

> > Maybe you could get into some trouble by force-removing modules at
> > various points, but you're in undefined behaviour generally at that
> > point, so I don't think there's much point going out-of-our-way to try
> > to support that.
>
> You can prevent that by refcounting the kunit module / symbols, by each test.
>

Again, I don't think KUnit is any more special than any other module
here. I don't think we need to do this ourselves, as it shouldn't be
possible to remove kunit without first removing any dependent modules.

Of course, happy to look into this again if anyone can come up with an
actual crash, but I'd rather get this fix in first. At the very least,
this patch shouldn't introduce any _new_ issues.

Cheers,
-- David
>   Luis

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Brendan Higgins]

On Thu, Jul 28, 2022 at 4:42 AM David Gow <davidgow@google.com> wrote:
>
> On Thu, Jul 21, 2022 at 4:29 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, Jul 20, 2022 at 05:26:02PM +0800, David Gow wrote:
> > > On Wed, Jul 20, 2022 at 1:58 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > >
> > > > On Wed, Jul 13, 2022 at 08:24:32AM -0700, Daniel Latypov wrote:
> > > > > On Tue, Jul 12, 2022 at 5:52 PM David Gow <davidgow@google.com> wrote:
> > > > > >
> > > > > > The new KUnit module handling has KUnit test suites listed in a
> > > > > > .kunit_test_suites section of each module. This should be loaded when
> > > > > > the module is, but at the moment this only happens if KUnit is built-in.
> > > > > >
> > > > > > Also load this when KUnit is enabled as a module: it'll not be usable
> > > > > > unless KUnit is loaded, but such modules are likely to depend on KUnit
> > > > > > anyway, so it's unlikely to ever be loaded needlessly.
> > > > >
> > > > > This seems reasonable to me.
> > > > >
> > > > > Question: what happens in this case?
> > > > > 1. insmod <test-module>
> > > > > 2. insmod kunit
> > > > > 3. rmmod <test-module>
> > > > >
> > > > > I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(),
> > > > > for <test-module>, I think?
> > > > > But we never called __kunit_test_suites_init().
> > > > > My fear is what breaks as a result of this precondition break.
> > >
> > > I don't think this should be possible: any module with KUnit tests
> > > will depend on the 'kunit' module (or, at least, kunit symbols), so
> > > shouldn't load without kunit already present.
> > >
> > > If modprobe is used, kunit will automatically be loaded. If insmod is
> > > used directly, loading the first module should error out with
> > > something like:
> > > [   82.393629] list_test: loading test module taints kernel.
> > > [   82.409607] list_test: Unknown symbol kunit_binary_ptr_assert_format (err -2)
> > > [   82.409657] list_test: Unknown symbol kunit_do_failed_assertion (err -2)
> > > [   82.409799] list_test: Unknown symbol kunit_binary_assert_format (err -2)
> > > [   82.409820] list_test: Unknown symbol kunit_unary_assert_format (err -2)
> > > insmod: ERROR: could not insert module
> > > /lib/modules/5.19.0-rc1-15284-g9ec67db0c271/kernel/lib/list-test.ko:
> > > Unknown symbol in module
> >
> > This can be fixed with a request_module() call. And since this is a
> > generic requirement, you can have the wrappers do it for you.
> >
>
> I'm not convinced that this is worth the trouble, particularly since
> KUnit needs to be loaded already before any test-specific code in a
> module is run. _Maybe_ we could put it in the code which looks for the
> .kunit_test_suites section, but even then it seems like a bit of an
> ugly hack.
>
> Personally, I'm not particularly concerned about test modules failing
> to load if KUnit isn't already present -- if people want all of a
> module's dependencies loaded, that's what modprobe is for.
>
> That being said, if you feel particularly strongly about it, this is
> something we can look at. Let's do so in a separate patch though: this
> one does fix a regression as-is.

I agree. We need a fix for 3d6e44623841 to go in for 5.20 - we've
gotten several complaints about it. This patch seems to accomplish
that.

I am not an expert on the module stuff by any means, so I am
absolutely open to continuing this discussion in a follow-up patch,
but I think we need this fix now.

If no one objects, I am going to ask Shuah to take this patch.

> > > Maybe you could get into some trouble by force-removing modules at
> > > various points, but you're in undefined behaviour generally at that
> > > point, so I don't think there's much point going out-of-our-way to try
> > > to support that.
> >
> > You can prevent that by refcounting the kunit module / symbols, by each test.
> >
>
> Again, I don't think KUnit is any more special than any other module
> here. I don't think we need to do this ourselves, as it shouldn't be
> possible to remove kunit without first removing any dependent modules.
>
> Of course, happy to look into this again if anyone can come up with an
> actual crash, but I'd rather get this fix in first. At the very least,
> this patch shouldn't introduce any _new_ issues.

Sounds good. I will send my Reviewed-by in a separate email, as per usual.

Cheers

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Luis Chamberlain]

On Wed, Jul 13, 2022 at 08:52:20AM +0800, David Gow wrote:
> The new KUnit module handling has KUnit test suites listed in a
> .kunit_test_suites section of each module. This should be loaded when
> the module is, but at the moment this only happens if KUnit is built-in.
> 

This commit log does not describe what functionality is broken exactly
without this commit. What functionality from kunit is provided when
.kunit_test_suites is available?

> Also load this when KUnit is enabled as a module: it'll not be usable
> unless KUnit is loaded, 

What benefit is there to load a kunit module without kunit?

> but such modules are likely to depend on KUnit
> anyway, so it's unlikely to ever be loaded needlessly.
>
> Fixes: 3d6e44623841 ("kunit: unify module and builtin suite definitions")
> Signed-off-by: David Gow <davidgow@google.com>

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Wed, Jul 20, 2022 at 1:57 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Jul 13, 2022 at 08:52:20AM +0800, David Gow wrote:
> > The new KUnit module handling has KUnit test suites listed in a
> > .kunit_test_suites section of each module. This should be loaded when
> > the module is, but at the moment this only happens if KUnit is built-in.
> >
>
> This commit log does not describe what functionality is broken exactly
> without this commit. What functionality from kunit is provided when
> .kunit_test_suites is available?
>

Sorry: the explanation is a bit obtuse, I admit.

Basically, when kunit itself is built as a module, no tests run. This
is because the code to load the .kunit_test_suites section is compiled
out if kunit is not built-in, but it should be present even if kunit
is built as a module.

> > Also load this when KUnit is enabled as a module: it'll not be usable
> > unless KUnit is loaded,
>
> What benefit is there to load a kunit module without kunit?
>

None whatsoever, and it should be impossible. This was just rationale
for the overhead of loading the section being likely insignificant,
but it's worded pretty poorly.

> > but such modules are likely to depend on KUnit
> > anyway, so it's unlikely to ever be loaded needlessly.
> >
> > Fixes: 3d6e44623841 ("kunit: unify module and builtin suite definitions")
> > Signed-off-by: David Gow <davidgow@google.com>

I'll update the commit message when I send a new version of this out.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Brendan Higgins]

On Tue, Jul 12, 2022 at 8:52 PM David Gow <davidgow@google.com> wrote:
>
> The new KUnit module handling has KUnit test suites listed in a
> .kunit_test_suites section of each module. This should be loaded when
> the module is, but at the moment this only happens if KUnit is built-in.
>
> Also load this when KUnit is enabled as a module: it'll not be usable
> unless KUnit is loaded, but such modules are likely to depend on KUnit
> anyway, so it's unlikely to ever be loaded needlessly.
>
> Fixes: 3d6e44623841 ("kunit: unify module and builtin suite definitions")
> Signed-off-by: David Gow <davidgow@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

[Geert Uytterhoeven]

Hi David,

On Wed, Jul 13, 2022 at 2:55 AM David Gow <davidgow@google.com> wrote:
> The new KUnit module handling has KUnit test suites listed in a
> .kunit_test_suites section of each module. This should be loaded when
> the module is, but at the moment this only happens if KUnit is built-in.
>
> Also load this when KUnit is enabled as a module: it'll not be usable
> unless KUnit is loaded, but such modules are likely to depend on KUnit
> anyway, so it's unlikely to ever be loaded needlessly.
>
> Fixes: 3d6e44623841 ("kunit: unify module and builtin suite definitions")
> Signed-off-by: David Gow <davidgow@google.com>

Thanks for your patch!

This fixes the regression where modular tests are no longer run
after loading them.
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

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