Message-ID: <CABi2SkUG8bhKQeHd_pvLw4y3ZY+Z8CvxZ_iV4YhTc+JQqe9TxA@mail.gmail.com>
Date: Fri, 18 Oct 2024 17:10:01 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Mark Brown <broonie@...nel.org>
Cc: Muhammad Usama Anjum <usama.anjum@...labora.com>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	akpm@...ux-foundation.org, linux-kselftest@...r.kernel.org, 
	linux-mm@...ck.org, linux-hardening@...r.kernel.org, 
	linux-kernel@...r.kernel.org, pedro.falcato@...il.com, willy@...radead.org, 
	vbabka@...e.cz, Liam.Howlett@...cle.com, rientjes@...gle.com, 
	keescook@...omium.org
Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

HI Mark

On Fri, Oct 18, 2024 at 2:05 PM Mark Brown <broonie@...nel.org> wrote:
>
> On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote:
> > On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@...nel.org> wrote:
> > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:
>
> > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
> > > line:2573" which automated systems aren't going to be able to associate
> > > with the passing "test_munmap_free_multiple_ranges_with_split", nor with
> > > any failures that occur on any other lines in the function.
>
> > I see. That will happen when those tests are modified and line number
> > changes. I could see reasoning for this argument, especially when
> > those tests are flaky and get updated often.
>
> > In practice, I hope any of those kernel self-test failures should get
> > fixed immediately, or even better, run before dev submitting the patch
> > that affects the mm area.
>
> That's not the entire issue - it is also a problem that the test name
> is not the same between passes and failures so automated systems can't
> associate the failures with the passes.
I failed to understand this part.
Maybe you meant the failing logging  is not the same across the
multiple versions of test code, by testname you meant "failing
logging"

>When a test starts failing they
> will see the passing test disappear and a new test appears that has never
> worked.
 > This will mean that for example if they have bisection support
> or UI for showing when a test started regressing those won't work.  The
> test name needs to be stable, diagnostics identifying why or where it
> failed should be separate prints.
>
If the test hasn't been changed for a while,  and start failing. Then
it is quite easy to run the same test on recent code changes. I think
you might agree with me on this. The only thing that bisec needs to
check is if the entire tests are failing or not.

I haven't used the biset functionality, so I'm not sure how it works
exactly, e.g. when it runs on the old version of kernel, does it use
the test binary from the old kernel ? or the test binary provided by
dev ?

> Actually, prompted by the comments below about test variants I've now
> run the test and see that what's in -next is also broken in that it's
> running a lot of the tests twice with sealing enabled or disabled but
> not including this in the reported test name resulting in most of the
> tests reporting like this:
>
>    ok 11 test_seal_mprotect
>    ok 12 test_seal_mprotect
>
> which is also going to confuse automated systems, they have a hard time
> working out which instance is which (generally the test numbers get
> ignored between runs as they're not at all stable).  The test names need
> to roll in the parameterisation:
>
>    ok 11 test_seal_mprotect seal=true
>    ok 12 test_seal_mprotect seal=false
>
> (or something, the specific format doesn't matter so long as the names
> are both stable and distinct).
>
Yes. Agree that this is a limitation of this macro.

> > Having line number does help dev to go to error directly, and I'm not
> > against filling in the "action" field, but you might also agree with
> > me, finding unique text for each error would require some decent
> > amount of time, especially for large tests such as mseal_test.
>
> In these situations if it's a typical Unix API function setting errno
> that failed I tend to end up writing diagnostics like:
>
>         ksft_perror("open()")
>
> possibly with some of the arguments included as well, or something
> equivalently basic for other kinds of error.  This is fairly mindless so
> quick and easy to do and more robust against line number slips if you're
> not looking at exactly the same version of the code, sometimes it's even
> enough you don't even need to look at the test to understand why it's
> upset.
>
I understand what you are saying, but personally, I still think line
numbers are a faster and more direct way to failure.

> > > Honestly this just sounds and looks like kselftest_harness.h, it's
> > > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
> > > for asserts.  The main gotchas with it are that it's not particularly
>
> > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture.
>
> > If I  switch to test_fixture, e,g, using TEST(test_name)
>
> > how do I pass the "seal" flag to it ?
> > e.g. how do I run the same test twice, first seal = true, and second seal=false.
>
> >         test_seal_mmap_shrink(false);
> >         test_seal_mmap_shrink(true);
>
> That looks like fixture variants to me, using those with
> kselftest_harness.h will also fix the problem with duplicate test names
> being used since it generates different names for each instance of the
> test.  Something like:
>
> FIXTURE(with_seal)
> {
> };
>
> FIXTURE_VARIANT(with_seal)
> {
>         bool seal;
> };
>
> FIXTURE_VARIANT_ADD(with_seal, yes)
> {
>         .seal = true,
> };
>
> FIXTURE_VARIANT_ADD(with_seal, no)
> {
>         .seal = false,
> };
>
> FIXTURE_SETUP(with_seal)
> {
> }
>
> FIXTURE_TEARDOWN(with_seal)
> {
> }
>
> then a bunch of tests using that fixture:
>
> TEST_F(with_seal, test_seal_mmap_shrink)
> {
>         if (variant->seal) {
>                 /* setup sealing */
>         }
>
>         ...
> }
>
> TEST_F(with_seal, test_seal_mprotect)
> {
>         if (variant->seal) {
>                 /* setup sealing */
>         }
>
>         ...
> }
>
> You don't need to actually set up anything in your fixture, but you do
> need to have setup and teardown functions so the framework can emit
> required boilerplate.  The gcs-locking.c test I recently added in -next
> is an example of a similar thing where we just need the variants,
> there's no actual fixture.

Thanks! This is really helpful, I think the existing mseal_test can be
quickly converted using this example.

(A side note: if selftest documentation is updated to include this
example, it will be much easier to future dev to follow)

Thanks
-Jeff

Powered by blists - more mailing lists