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@xxxxxxxxxx> 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@xxxxxxxxxxxx wrote:
> > > > From: Jeff Xu <jeffxu@xxxxxxxxxxxx>
> > > >
> > > > 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.
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.
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.
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!
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@xxxxxxxxxxxx>
> > > > ---
> > > > 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
> > > >
