Message-ID: <CABi2SkUgDZtJtRJe+J9UNdtZn=EQzZcbMB685P=1rR7DUhg=6Q@mail.gmail.com>
Date: Fri, 13 Sep 2024 15:50:00 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc: 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, 
	broonie@...nel.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 Lorenzo

On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
<lorenzo.stoakes@...cle.com> wrote:
>
> On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
> > On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes
> > <lorenzo.stoakes@...cle.com> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> > > > On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@...omium.org wrote:
> > > > > From: Jeff Xu <jeffxu@...omium.org>
> > > > >
> > > > > Add sealing test to cover mmap for
> > > > > Expand/shrink across sealed vmas (MAP_FIXED)
> > > > > Reuse the same address in !MAP_FIXED case.
>
> Hi Jeff, I really want to find a constructive way forward, but you've
> basically ignored more or less everything I've said here.
>
> I could respond again to each of your points here, but - from my point of
> view - if your response to 'what is this even testing?' is to just repeat
> in effect the name of the test - we will be stuck in a loop, which will be
> exited with a NACK. I don't want this.
>
> The majority of these tests, from a VMA/mmap point of view, appear to me to
> be essentially testing 'does basic mmap functionality work correctly',
> which isn't testing mseal.
>
> Look - I appreciate your commitment to testing (see my work on mm/vma.c - I
> care passionately about testing) - but we must make sure we are actually
> testing what we mean to.
>
I'm also passionate about testing :-)

Maybe there is a mis-understanding ? I explained the purpose of this
patch set in various responses, maybe not directly to your email though.

Even though the number of lines is large in these patches, its main
intention is to test Pedro's in-place change (from can_modify_mm to
can_modify_vma). Before this patch,  the test had a common pattern:
setup memory layout, seal the memory, perform a few mm-api steps, verify
return code (not zero).  Because of the nature of out-of-loop,  it is
sufficient to just verify the error code in a few cases.

With Pedro's in-loop change, the sealing check happens later in the
stack, thus there are more things and scenarios to verify. And there were
feedback to me during in-loop change that selftest should be extensive
enough to discover all regressions.  Even though this viewpoint is subject
to debate. Since none would want to do it, I thought I would just do it.

So the Patch V3 1/5 is dedicated entirely to increasing the verification
for existing scenarios, this including checking return code code, vma-size,
etc after mm api return.

Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop
change, we discovered a bug in unmap(), and unmap() is not atomic.
This leads to 4/5(mmap), 5/5(mremap), which calls munmap().
In addition, I add scenarios to cover cross-multiple-vma cases.

The  high-level goal of mseal test are two folds:
1> make sure sealing is working correctly under different scenarios,
i.e. sealed mapping are not modified.
2> For unsealed memory, added mseal code  doesn't regress on regular mm API.

The goal 2 is as important as 1, that is why tests usually are done in
two phases, one with sealing, the other without.

> So I suggest as a constructive way forward - firstly, submit a regression
> test for the change Liam wrapped into his series regarding the -EPERM
> thing.
>
I could work on this (to split the patch further) if this helps
acceptance of the patch series.

However, since the merge window is closer, everyone is busy, and it
is not really urgent to get it merged.  the added tests  already
passed in the linux-next branch,  we could wait till after
merge-window to review/perfect those tests.

> This should go in uncontroversially, I will take the time to review it and
> I don't see why that shouldn't be relatively straight forward. I will drop
> the concerns about things like test macros etc. for that.
>
> Then after that, I suggest we have a discussion about - at a higher level -
> what it is you want to test. And then between me, you, Liam and Pedro -
> ahead of time, list out the tests that we want, with all of us reaching
> consensus.
>
> I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point
> too - I may be missing something, but I cannot for the life me understand
> why we have to assert negations only, and other self tests do not do this.
>
My most test-infra related comments comes from Muhammad Usama Anjum
(added into this email), e.g. assert is not recommended.[1] ,

[1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/

> I have replied to a few sample points below.
>
> All of us simply want to help make sure mseal works as well as it can, this
> is the only motivation at play here.
>
> Hope you have a great weekend!
>

Thanks
Hope a great weekend too !
-Jeff


-Jeff

> Cheers, Lorenzo
>
> > > >
> > > > This commit message is woefully small. I told you on v1 to improve the
> > > > commit messages. Linus has told you to do this before.
> > > >
> > > > Please actually respond to feedback. Thanks.
> > > >
> > > > >
> > > > > Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> > > > > ---
> > > > >  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
> > > > >  1 file changed, 125 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > > > > index e855c8ccefc3..3516389034a7 100644
> > > > > --- a/tools/testing/selftests/mm/mseal_test.c
> > > > > +++ b/tools/testing/selftests/mm/mseal_test.c
> > > > > @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
> > > > >     REPORT_TEST_PASS();
> > > > >  }
> > > > >
> > > > > +static void test_seal_mmap_expand_seal_middle(bool seal)
> > > >
> > > > This test doesn't expand, doesn't do anything in the middle. It does mmap()
> > > > though and relates to mseal, so that's something... this is compeltely
> > > > misnamed and needs to be rethought.
> > > >
> > >
> > > OK correction - it _seals_ in the middle. The remained of the criticism remains,
> > > and this is rather confusing... and I continue to wonder what the purpose of
> > > this is?
> > >
> > It expands the size (start from ptr).
> >
> > > > > +{
> > > > > +   void *ptr;
> > > > > +   unsigned long page_size = getpagesize();
> > > > > +   unsigned long size = 12 * page_size;
> > > > > +   int ret;
> > > > > +   void *ret2;
> > > > > +   int prot;
> > > > > +
> > > > > +   setup_single_address(size, &ptr);
> > > >
> > > > Please replace every single instance of this with an mmap(). There's
> > > > literally no reason to abstract it. And munmap() what you map.
> > > >
> > No, we need to abstract it.  In addition to the mmap, it also
> > allocates an additional two blocks before and after the allocated
> > memory, to avoid auto-merging, so we can use get_vma_size.
>
> It doesn't?
>
> static void setup_single_address(int size, void **ptrOut)
> {
>         void *ptr;
>
>         ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>         *ptrOut = ptr;
> }
>
> >
> > > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > >
> > > > Pretty sure Pedro pointed out you should be checking against MAP_FAILED
> > > > here. I really don't understand why the rest of your test is full of
> > > > mmap()'s but for some reason you choose to abstract this one call? What?
> > > >
> > > > > +   /* ummap last 4 pages. */
> > > > > +   ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
> > > >
> > > > sys_munmap()? What's wrong with munmap()?
> > > >
> > > > > +   FAIL_TEST_IF_FALSE(!ret);
> > > >
> > > > Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
> > > >
> > > > Would be nice to have something human-readable like ASSERT_EQ() or
> > > > ASSERT_TRUE() or ASSERT_FALSE().
> > > >
> > ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The
> > FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks
> > related to self-test infra, such as count how many tests are failing.
>
> Can you please point me to where it says you should implement your own
> macro that only tests the negation of an expression?
>
> I have found other self tests that do.
>
> >
> > > > > +
> > > > > +   size = get_vma_size(ptr, &prot);
> > > > > +   FAIL_TEST_IF_FALSE(size == 8 * page_size);
> > > > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +   if (seal) {
> > > > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > > > +   }
> > > > > +
> > > > > +   /* use mmap to expand and overwrite (MAP_FIXED)  */
> > > >
> > > > You don't really need to say MAP_FIXED, it's below.
> > > >
> > Adding a comment here to help reviewers.
> >
> > > > > +   ret2 = mmap(ptr, 12 * page_size, PROT_READ,
> > > > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > > >
> > > > Why read-only?
> > > >
> > > > You're not expanding you're overwriting. You're not doing anything in the
> > > > middle.
> > > >
> > The MAP_FIXED is overwriting.  It also expands the address range
> > (start from ptr) from 8 to 12 pages.
> >
> > > > I'm again confused about what you think you're testing here. I don't think
> > > > we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten
> > > > VMA?
> > > >
> > > > You just need a single instance of a MAP_FIXED mmap() over a sealed mmap()
> > > > if that's what you want.
> > > >
> > > > > +   if (seal) {
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > > > +
> > > > > +           size = get_vma_size(ptr, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +   } else
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > > >
> > > > Don't do dangling else's after a big block.
> > > >
> > patch passed the checkpatch.pl for style check.
> >
> > > > > +
> > > > > +   REPORT_TEST_PASS();
> > > > > +}
> > > > > +
> > > > > +static void test_seal_mmap_shrink_seal_middle(bool seal)
> > > >
> > > > What's going on in the 'middle'? This test doesn't shrink, it overwrites
> > > > the beginning of a sealed VMA?
> > >
> > > Correction - the middle is sealed. Other points remain.
> > >
> > The mmap attempts to shrink the address range from 12 pages to 8 pages.
> >
> > > > > +{
> > > > > +   void *ptr;
> > > > > +   unsigned long page_size = getpagesize();
> > > > > +   unsigned long size = 12 * page_size;
> > > > > +   int ret;
> > > > > +   void *ret2;
> > > > > +   int prot;
> > > > > +
> > > > > +   setup_single_address(size, &ptr);
> > > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > > +
> > > > > +   if (seal) {
> > > > > +           ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > > > +   }
> > > > > +
> > > > > +   /* use mmap to shrink and overwrite (MAP_FIXED)  */
> > > >
> > > > What exactly are you shrinking? You're overwriting the start of the vma?
> > > >
> > > > What is this testing that is different from the previous test? This seems
> > > > useless honestly.
> > > >
> > Again, as above, one test is expanding, the other test is shrinking.
> > Please take a look at mmap parameters and steps before mmap call.
> >
> >
> > > > > +   ret2 = mmap(ptr, 7 * page_size, PROT_READ,
> > > > > +                   MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
> > > > > +   if (seal) {
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > > > +           FAIL_TEST_IF_FALSE(errno == EPERM);
> > > > > +
> > > > > +           size = get_vma_size(ptr, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > >
> > > > What the hell is this comparison to magic numbers? This is
> > > > ridiculous. What's wrong with PROT_xxx??
> > > >
> > The PROT_xxx can't be used here.
> > get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.
> >
> > > > > +
> > > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +           size = get_vma_size(ptr + 4 * page_size, &prot);
> > > > > +           FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > > > > +           FAIL_TEST_IF_FALSE(prot == 0x4);
> > > >
> > > > Err dude, you're doing this twice?
> > > >
> > The second get_vma_size should be (ptr + 8 * page_size)
> > I will update that.
> >
> > > > So what are we testing here exactly? That we got a VMA split? This is
> > > > err... why are we asserting this?
> > >
> > > I guess, that we can't overwrite a sealed bit of a VMA at the end. But again
> > > this feels entirely redundant. For this kind of thing to fail would mean the
> > > whole VMA machinery is broken.
> > >
> > The test is testing mmap(MAP_FIXED), since it can be used to overwrite
> > the sealed memory range (without sealing), then there is a variant of
> > expand/shrink.
> >
> >
> > > >
> > > > > +   } else
> > > > > +           FAIL_TEST_IF_FALSE(ret2 == ptr);
> > > > > +
> > > > > +   REPORT_TEST_PASS();
> > > > > +}
> > > > > +
> > > > > +static void test_seal_mmap_reuse_addr(bool seal)
> > > >
> > > > This is wrong, you're not reusing anything. This test is useless.
> > > >
> > The ptr is reused as a hint.
> >
> > > > > +{
> > > > > +   void *ptr;
> > > > > +   unsigned long page_size = getpagesize();
> > > > > +   unsigned long size = page_size;
> > > > > +   int ret;
> > > > > +   void *ret2;
> > > > > +   int prot;
> > > > > +
> > > > > +   setup_single_address(size, &ptr);
> > > > > +   FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > > +
> > > > > +   if (seal) {
> > > > > +           ret = sys_mseal(ptr, size);
> > > > > +           FAIL_TEST_IF_FALSE(!ret);
> > > >
> > > > We could avoid this horrid ret, ret2 naming if you just did:
> > > >
> > > >       FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
> > > >
> > > > > +   }
> > > > > +
> > > > > +   /* use mmap to change protection. */
> > > > > +   ret2 = mmap(ptr, size, PROT_NONE,
> > > > > +                   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > > >
> > > > How are you using mmap to change the protection when you're providing a
> > > > hint to the address to use? You're not changing any protection at all!
> > > >
> > It is necessary to add the this tests to make sure mseal is behave as
> > it should be, which is !MAP_FIXED case, new address will be allocated,
> > instead of fail of mmap()
> >
> >
> > > > You're allocating an entirely new VMA hinting that you want it near
> > > > ptr. Please read the man page for mmap():
> > > >
> > > >        If addr is NULL, then the kernel chooses the (page-aligned) address
> > > >        at which to create the mapping; this is the most portable method of
> > > >        creating a new mapping.  If addr is not NULL, then the kernel takes
> > > >        it as a hint about where to place the mapping; on Linux, the kernel
> > > >        will pick a nearby page boundary (but always above or equal to the
> > > >        value specified by /proc/sys/vm/mmap_min_addr) and attempt to create
> > > >        the mapping there.  If another mapping already exists there, the
> > > >        kernel picks a new address that may or may not depend on the hint.
> > > >        The address of the new mapping is returned as the result of the
> > > >        call.
> > > >
> > > > > +
> > > > > +   /* MAP_FIXED is not used, expect new addr */
> > > > > +   FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
> > > >
> > > > This is beyond horrible. You really have to add more asserts.
> > > >
> > Again assert is not recommended by self_test
> >
> > > > Also you're expecting a new address here, so again, what on earth are you
> > > > asserting? That we can mmap()?
> > > >
> > > > > +   FAIL_TEST_IF_FALSE(ret2 != ptr);
> > > > > +
> > > > > +   size = get_vma_size(ptr, &prot);
> > > > > +   FAIL_TEST_IF_FALSE(size == page_size);
> > > > > +   FAIL_TEST_IF_FALSE(prot == 0x4);
> > > > > +
> > > > > +   REPORT_TEST_PASS();
> > > > > +}
> > > > > +
> > > > >  int main(int argc, char **argv)
> > > > >  {
> > > > >     bool test_seal = seal_support();
> > > > > @@ -2243,7 +2360,7 @@ int main(int argc, char **argv)
> > > > >     if (!get_vma_size_supported())
> > > > >             ksft_exit_skip("get_vma_size not supported\n");
> > > > >
> > > > > -   ksft_set_plan(91);
> > > > > +   ksft_set_plan(97);
> > > >
> > > > I'm guessing this is the number of tests, but I mean this is horrible. Is
> > > > there not a better way of doing this?
> > > >
> > Again, this is recommended by self-test.
> >
> >
> >
> > > > >
> > > > >     test_seal_addseal();
> > > > >     test_seal_unmapped_start();
> > > > > @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
> > > > >     test_munmap_free_multiple_ranges(false);
> > > > >     test_munmap_free_multiple_ranges(true);
> > > > >
> > > > > +   test_seal_mmap_expand_seal_middle(false);
> > > > > +   test_seal_mmap_expand_seal_middle(true);
> > > > > +   test_seal_mmap_shrink_seal_middle(false);
> > > > > +   test_seal_mmap_shrink_seal_middle(true);
> > > > > +   test_seal_mmap_reuse_addr(false);
> > > > > +   test_seal_mmap_reuse_addr(true);
> > > > > +
> > > > >     ksft_finished();
> > > > >  }
> > > > > --
> > > > > 2.46.0.469.g59c65b2a67-goog
> > > > >

Powered by blists - more mailing lists