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@xxxxxxxxxx> wrote:
>
> On Thu, Jul 21, 2022 at 4:29 AM Luis Chamberlain <mcgrof@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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