Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 9 May 2022 13:43:40 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     David Gow <davidgow@google.com>
Cc:     Daniel Latypov <dlatypov@google.com>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Lucas De Marchi <lucas.demarchi@intel.com>,
        Aaron Tomlin <atomlin@redhat.com>,
        linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
        Brendan Higgins <brendanhiggins@google.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        "Guilherme G . Piccoli" <gpiccoli@igalia.com>,
        Sebastian Reichel <sre@kernel.org>,
        John Ogness <john.ogness@linutronix.de>,
        Joe Fradley <joefradley@google.com>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jani Nikula <jani.nikula@linux.intel.com>
Subject: Re: [PATCH v2] kunit: Taint kernel if any tests run
Message-ID: <Ynl8/MqmDZzpbh5y@bombadil.infradead.org>
References: <Ym7P7mCoMiQq99EM@bombadil.infradead.org>
 <Ym7QXOMK3fLQ+b6t@bombadil.infradead.org>
 <CABVgOSmXyN3SrDkUt4y_TaKPvEGVJgbuE3ycrVDa-Kt1NFGH7g@mail.gmail.com>
 <YnKS3MwNxvEi73OP@bombadil.infradead.org>
 <CAGS_qxrz1WoUd5oGa7p1-H2mQVbkRxSTEbqnCG=aBj=xnMu1zQ@mail.gmail.com>
 <YnLJ6dJQBTYjBRHZ@bombadil.infradead.org>
 <CAGS_qxoFECVJD3Jby1eTWG741hBWuotuEM78PU-qfyvp-nLV7Q@mail.gmail.com>
 <YnLsPgbQ7CHiannN@bombadil.infradead.org>
 <YnNnLIZDxkNwECv+@bombadil.infradead.org>
 <CABVgOS=8=41KgVEgRAGcDZ_JrZpsVaK24ca0jR5J74XY9GCmDA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CABVgOS=8=41KgVEgRAGcDZ_JrZpsVaK24ca0jR5J74XY9GCmDA@mail.gmail.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Fri, May 06, 2022 at 03:01:34PM +0800, David Gow wrote:
> On Thu, May 5, 2022 at 1:57 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, May 04, 2022 at 02:12:30PM -0700, Luis Chamberlain wrote:
> > > On Wed, May 04, 2022 at 02:19:59PM -0500, Daniel Latypov wrote:
> > > > On Wed, May 4, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > > > OK so, we can just skip tainting considerations for selftests which
> > > > > don't use modules for now. There may be selftests which do wonky
> > > > > things in userspace but indeed I agree the userspace taint would
> > > > > be better for those but I don't think it may be worth bother
> > > > > worrying about those at this point in time.
> > > > >
> > > > > But my point in that sharing a taint between kunit / selftests modules
> > > > > does make sense and is easily possible. The unfortunate aspect is just
> > > >
> > > > Yes, I 100% agree that we should share a taint for kernelspace testing
> > > > from both kunit/kselftest.
> > > > Someone running the system won't care what framework was used.
> > >
> > > OK do you mind doing the nasty work of manually adding the new
> > > MODULE_TAINT() to the selftests as part of your effort?
> > >
> > > *Alternatively*, if we *moved* all sefltests modules to a new
> > > lib/debug/selftests/ directory or something like that then t would
> > > seem modpost *could* add the taint flag automagically for us without
> > > having to edit or require it on new drivers. We have similar type of
> > > taint for staging, see add_staging_flag().
> > >
> > > I would *highly* prefer this approach, event though it is more work,
> > > because I think this is a step we should take anyway.
> > >
> > > However, I just checked modules on lib/ and well, some of them are
> > > already in their own directory, like lib/math/test_div64.c. So not
> > > sure, maybe just move a few modules which are just in lib/*.c for now
> > > and then just sprinkle the MODULE_TAINT() to the others?
> >
> > I *think* we could just pull this off with a much easier approach,
> > simply looking for the substrings in the module name in modpost.c:
> >
> >   * "_test." || "-test."
> >   * ^"test_" || ^"test-"
> >
> > An issue with this of course is a vendor $FOO with an out of tree
> > test driver may end up with the taint. Perhaps we don't care.
> >
> > That means moving selftests to its own directory is not needed at this
> > point in time.
> 
> I can't say I'm thrilled with the idea of just doing name comparisons,
> particularly since not all of them match this pattern, for example:
> bpf_testmod.ko. (Though, frankly, more of them do than I'd've
> guessed.)
> 
> Maybe adding a taint call to the selftest helper module framework in
> kselftest_module.h, though again, there are several tests which don't
> use it.

Right, I can't think of a generic way to peg this. I think long term
we do stand to gain to move all selftests under a lib/debug/selftests/
or something like that, but for now what I suggested is the only thing
I can come up with.

> I _suspect_ we'd be able to hit most of them by tainting in frameworks
> like the above, and patch the remaining modules manually.

Works with me.

> There's also
> definitely a grey area with things like netdevsim, which are used a
> lot as helper modules by selftests, but may have other uses as well.

They can peg the module if they want the taint.

> (The advantage of the KUnit tainting is that, due to KUnit's
> centralised executor, we can be sure all KUnit tests will correctly
> trigger the taint. But maybe it doesn't matter as much if one or two
> selftests miss out.)

That is what I was thinking.

I'm convinced we *should* move selftests to a one directory. The
amount of stuff in lib/ is getting out of hand.

  Luis