linux-kernel - Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSkpT2kqVec2F7BsTF5tyABO43bseETC2Dz238zN+sTfQw@mail.gmail.com>
Date:   Wed, 20 Jul 2022 17:26:02 +0800
From:   David Gow <davidgow@...gle.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Daniel Latypov <dlatypov@...gle.com>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Jeremy Kerr <jk@...econstruct.com.au>,
        linux-modules@...r.kernel.org,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m

On Wed, Jul 20, 2022 at 1:58 AM Luis Chamberlain <mcgrof@...nel.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@...gle.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

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