Re: [RFC PATCH v1 1/1] selftest/mm: refactor mseal_test

[Jeff Xu]

Hi Lorenzo,

On Thu, Jan 2, 2025 at 9:30 AM Lorenzo Stoakes
<lorenzo.stoakes@xxxxxxxxxx> wrote:
>
> Sorry for delay, was super busy leading up to xmas break, then had ~2.5
> weeks off.
>
Thanks for reviewing.  There are lots of comments, so it takes some
time to go through comments and experiment with some of the suggested
changes. Please see below.

> And happy new year! :)
>
Happy new year !

> On Wed, Dec 11, 2024 at 05:33:11AM +0000, jeffxu@xxxxxxxxxxxx wrote:
> > From: Jeff Xu <jeffxu@xxxxxxxxxxxx>
> >
> > This change creates the initial version of memorysealing.c.
> >
> > The introduction of memorysealing.c, which replaces mseal_test.c and
>
> I mean I don't want to be a pain but I would kinda prefer to have 'mseal'
> everywhere mseal is to avoid confusion vs. memfd seals.
>
As I tried to explain, the memorysealing.c will eventually replace
mseal_test.c, after all the tests are moved to use kselftest_karness.
How about mseal_test_new.c ? I'm open to new names if you have one.

> Of course we in the kernel absolutely _love_ to overload the meaning of terms
> but some traditions are worth breaking :)
>
> > uses the kselftest_harness, aims to initiate a discussion on using the
> > selftest harness for memory sealing tests. Upon approval of this
> > approach, the migration of tests from mseal_test.c to memorysealing.c
> > can be implemented in a step-by-step manner.
>
> Well, I for one like this approach so (unsurprisingly) :) but perhaps
> sensible to do it iteratively so we can also tweak things as we go.
>
Yes, that is the idea.

> >
> > This tests addresses following feedback from previous reviews:
> >
> > 1> Use kselftest_harness instead of custom macro, such as EXPECT_XX,
> > ASSERT_XX, etc.  (Lorenzo Stoakes, Mark Brown) [1]
> >
> > 2> Use MAP_FAILED to check the return of mmap (Lorenzo Stoakes).
>
> Thanks!
>
> >
> > 3>  Adding a check for vma size and prot bits. The discussion for
> >     this can be found in [2] [3], here is a brief summary:
> >     This is to follow up on Pedro’s in-loop change (from
> >     can_modify_mm to can_modify_vma). When mseal_test is initially
> >     created, they have 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
> >     was feedbacks to me that mseal_test should be extensive enough to
> >     discover all regressions. Hence I'm adding check for vma size and prot
> >     bits.
> >
> > In this change: we created two fixtures:
> >
> > Fixture basic:   This creates a single VMA, the VMA has a
> > PROT_NONE page at each end to prevent auto-merging.
> >
> > Fixture wo_vma: Two VMAs back to end, a PROT_NONE page at each
> > end to prevent auto-merging.
> >
> > In addition, I add one test (mprotec) in each fixture to demo the tests.
> >
> > [1] https://lore.kernel.org/all/20240830180237.1220027-5-jeffxu@xxxxxxxxxxxx/
> > [2] https://lore.kernel.org/all/CABi2SkUgDZtJtRJe+J9UNdtZn=EQzZcbMB685P=1rR7DUhg=6Q@xxxxxxxxxxxxxx/
> > [3] https://lore.kernel.org/all/2qywbjb5ebtgwkh354w3lj3vhaothvubjokxq5fhyri5jeeton@duqngzo3swjz/
> >
> > Signed-off-by: Jeff Xu <jeffxu@xxxxxxxxxxxx>
> > ---
> >  tools/testing/selftests/mm/.gitignore      |   1 +
> >  tools/testing/selftests/mm/Makefile        |   1 +
> >  tools/testing/selftests/mm/memorysealing.c | 182 +++++++++++++++++++++
> >  tools/testing/selftests/mm/memorysealing.h | 116 +++++++++++++
> >  tools/testing/selftests/mm/mseal_test.c    |  67 +-------
> >  5 files changed, 301 insertions(+), 66 deletions(-)
> >  create mode 100644 tools/testing/selftests/mm/memorysealing.c
> >  create mode 100644 tools/testing/selftests/mm/memorysealing.h
> >
> > diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> > index a51a947b2d1d..982234a99f20 100644
> > --- a/tools/testing/selftests/mm/.gitignore
> > +++ b/tools/testing/selftests/mm/.gitignore
> > @@ -57,3 +57,4 @@ hugetlb_dio
> >  pkey_sighandler_tests_32
> >  pkey_sighandler_tests_64
> >  guard-pages
> > +memorysealing
> > diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> > index 93a46ac633df..08876624f46d 100644
> > --- a/tools/testing/selftests/mm/Makefile
> > +++ b/tools/testing/selftests/mm/Makefile
> > @@ -67,6 +67,7 @@ TEST_GEN_FILES += map_populate
> >  ifneq (,$(filter $(ARCH),arm64 riscv riscv64 x86 x86_64))
> >  TEST_GEN_FILES += memfd_secret
> >  endif
> > +TEST_GEN_FILES += memorysealing
> >  TEST_GEN_FILES += migration
> >  TEST_GEN_FILES += mkdirty
> >  TEST_GEN_FILES += mlock-random-test
> > diff --git a/tools/testing/selftests/mm/memorysealing.c b/tools/testing/selftests/mm/memorysealing.c
> > new file mode 100644
> > index 000000000000..e10032528b64
> > --- /dev/null
> > +++ b/tools/testing/selftests/mm/memorysealing.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +#define _GNU_SOURCE
> > +#include "../kselftest_harness.h"
> > +#include <asm-generic/unistd.h>
> > +#include <errno.h>
> > +#include <syscall.h>
> > +#include "memorysealing.h"
> > +
> > +/*
> > + * To avoid auto-merging, create a VMA with PROT_NONE pages at each end.
> > + * If unsuccessful, return MAP_FAILED.
> > + */
> > +static void *setup_single_address(int size, int prot)
>
> Nitty, but size should really be an unsigned long.
>
sure, thanks

> > +{
> > +     int ret;
> > +     void *ptr;
> > +     unsigned long page_size = getpagesize();
>
> Trivial, but we store page size in self->page_size so why not pass it as a
> parameter instead of looking up?
>
sure.

> > +
> > +     ptr = mmap(NULL, size + 2 * page_size, prot,
> > +             MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > +
> > +     if (ptr == MAP_FAILED)
> > +             return MAP_FAILED;
> > +
> > +     /* To avoid auto-merging, change to PROT_NONE at each end. */
> > +     ret = sys_mprotect(ptr, page_size, PROT_NONE);
> > +     if (ret != 0)
> > +             return MAP_FAILED;
> > +
> > +     ret = sys_mprotect(ptr + size + page_size, page_size, PROT_NONE);
> > +     if (ret != 0)
> > +             return MAP_FAILED;
> > +
> > +     return ptr + page_size;
> > +}
>
> This could be done more easily with a single PROT_NONE, like:
>
>         static void *setup_single_address(unsigned long size, int prot,
>                         unsigned long page_size)
>         {
>                 void *ptr;
>
>                 /* Ensure we only merge what we intend to. */
>                 ptr = mmap(NULL, size + 2 * page_size, PROT_NONE,
>                            MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>                 if (ptr == MAP_FAILED)
>                         return MAP_FAILED;
>
>                 return mmap(&ptr[page_size], size, prot,
>                             MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>         }
>
sure, create PROT_NONE mapping first, then use mprotect to change the
middle part.

> It's probably worth having an equivalent freeing function to avoid leaks:
>
>         static int free_single_address(void *ptr, unsigned long size,
>                         unsigned long page_size)
>         {
>                 /* Also free the PROT_NONE sentinels. */
>                 return munmap(ptr - page_size, size + 2 * page_size);

[resp 1] could you clarify? I think we won't need freeing because
mapping will be sealed. You might recall, mseal() blocks munmap(), and
munmap() is atomic,  so the above line that you suggested will result
in no-action during FIXTURE_TEARDOWN. There will be separate test
cases to test munmap under sealing. In addition, the virtual address
space is big enough for this test complete without worry about the
out-of-memory situation. Do I miss anything ?

>         }
>
> > +
> > +FIXTURE(basic)
> > +{
> > +     unsigned long page_size;
> > +     unsigned long size;
> > +     void *ptr;
> > +};
> > +
> > +/*
> > + * Setup for basic:
> > + * Single VMA with 4 pages, prot = (PROT_READ | PROT_WRITE).
> > + */
> > +FIXTURE_SETUP(basic)
> > +{
> > +     int prot;
> > +
> > +     self->page_size = getpagesize();
> > +
> > +     if (!mseal_supported())
> > +             SKIP(return, "mseal is not supported");
> > +
> > +     /* Create a single VMA with 4 pages, prot as PROT_READ | PROT_WRITE. */
> > +     self->size = self->page_size * 4;
> > +     self->ptr = setup_single_address(self->size, PROT_READ | PROT_WRITE);
> > +     EXPECT_NE(self->ptr, MAP_FAILED);
> > +
> > +     EXPECT_EQ(self->size, get_vma_size(self->ptr, &prot));
>
> I mean fine but this check seems a bit overkill here, we don't need to
> check to ensure that PROT_NONE works.
>
[resp 2] Those checks are there,  for the same reason that test cases
usually check the return value of mmap(), mprotect() in the
FIXTURE_SETUP. You are right that those calls are unlikely to fail,
however, test cases generally do those  "housekeeping tasks"  to make
sure everything is OK before  the actual test. If you don't want to
keep those, what is the downside of keeping them ?

> > +     EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
>
> Nitty, but this seems a bit 'Yoda style', that is placing the expectation
> prior to the value whose expectation we are assessing, that is perhaps it'd
> be clearer as:
>
>         EXPECT_EQ(prot, PROT_READ | PROT_WRITE);
>
[resp 3] Both styles are used in self tests and I believe it is
equally clear,  I can accommodate this request for this patch.

> And again, I think this is really overkill here, if the kernel is unable to
> do an ordinary mapping correctly we have bigger problems than an mseal bug!
>
please see [resp 2]

> > +};
> > +
> > +FIXTURE_TEARDOWN(basic)
> > +{
>
> Probably worth adding munmap() teardown here.
>
please see [resp 1]

> > +}
> > +
> > +FIXTURE(two_vma)
> > +{
> > +     unsigned long page_size;
> > +     unsigned long size;
> > +     void *ptr;
> > +};
> > +
> > +/*
> > + * Setup for two_vma:
> > + * Two consecutive VMAs, each with 2 pages.
> > + * The first VMA:  prot = PROT_READ.
> > + * The second VMA: prot = (PROT_READ | PROT_WRITE).
> > + */
> > +FIXTURE_SETUP(two_vma)
> > +{
> > +     int prot;
> > +     int ret;
> > +
> > +     self->page_size = getpagesize();
> > +
> > +     if (!mseal_supported())
> > +             SKIP(return, "mseal is not supported");
> > +
> > +     /* Create a single VMA with 4 pages, prot as PROT_READ | PROT_WRITE. */
> > +     self->size = getpagesize() * 4;
> > +     self->ptr = setup_single_address(self->size, PROT_READ | PROT_WRITE);
> > +     EXPECT_NE(self->ptr, MAP_FAILED);
> > +
> > +     /* Use mprotect to split as two VMA. */
> > +     ret = sys_mprotect(self->ptr, self->page_size * 2, PROT_READ);
> > +     ASSERT_EQ(ret, 0);
> > +
>
> > +     /* Verify the first VMA is 2 pages and prot bits */
> > +     EXPECT_EQ(self->page_size * 2, get_vma_size(self->ptr, &prot));
> > +     EXPECT_EQ(PROT_READ, prot);
> > +
> > +     /* Verify the second VMA is 2 pages and prot bits */
> > +     EXPECT_EQ(self->page_size * 2,
> > +             get_vma_size(self->ptr + self->page_size * 2, &prot));
> > +     EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
>
> Again, as with the prior fixture, it seems a bit overkill, as this is
> essentially testing 'does mmap() and mprotect()' work. We should hope so :)
Please see [resp 2]

> > +};
> > +
> > +FIXTURE_TEARDOWN(two_vma)
> > +{
>
> Same comment as for the basic fixture - let's clean up after ourselves.
>
please see [resp 1]

> > +}
> > +
> > +/*
> > + * Verify mprotect is blocked.
> > + */
> > +TEST_F(basic, mprotect_basic)
> > +{
> > +     int ret;
> > +     unsigned long size;
> > +     int prot;
> > +
> > +     /* Seal the mapping. */
> > +     ret = sys_mseal(self->ptr, self->size, 0);
> > +     ASSERT_EQ(ret, 0);
> > +
> > +     /* Verify mprotect is blocked. */
> > +     ret = sys_mprotect(self->ptr, self->size, PROT_READ);
> > +     EXPECT_GT(0, ret);
>
> Yeah this one is kinda egregious, this is genuinely hard to read. Wouldn't
> this be better as:
>
>         EXPECT_NE(ret, 0);
>
> As it explicitly says 'not equal to a success case'. Rather than 'wait 0 is
> _greater than_ the result which means the result is _less than_ 0 which
> means an error but let me check the man page can it be positive, oh ok it's
> -1 on error, 0 on success', which was the Lorenzo thought process... :)
>
[resp 4] The man page says: "On success, mprotect() and
pkey_mprotect() return zero.  On error, these system calls return -1,
and errno is set to indicate the error."

I wll switch EXPECT_EQ(ret, -1) which is precisely what the man page says.

> > +     EXPECT_EQ(EPERM, errno);
>
> Let's invert these, Yoda style isn't really useful here and is a little
> harder to read, e.g. we should do:
>
>         EXPECT_EQ(errno, EPERM);
>
please see [resp 3]

> > +
> > +     /* Verify the VMA (sealed) isn't changed */
> > +     size = get_vma_size(self->ptr, &prot);
> > +     EXPECT_EQ(self->size, size);
> > +     EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
>
> Again, no to Yoda :)
>
please see [resp 3]

> I also wonder how useful this is - I guess belts + braces, and linux _can_
> do partial failures, but is that something we would care about in practice?
>
[resp 5]
Those checks will help to detect regression, such as  when Pedro
Falcato implemented in-loop change [1]. Liam R. Howlett also commented
that mseal_test should be extensive enough to check for size and prot
to avoid future regression [2]

For your question about partial failure, there will be additional
tests for partial-sealed-address range, and those will check the
sealed mapping only.

> > +}
> > +
> > +/*
> > + * Seal both VMAs in one mseal call.
> > + * Verify mprotect is blocked on both VMAs in various cases.
> > + */
> > +TEST_F(two_vma, mprotect)
>
> I'm guessing you aren't necessarily covering _all_ mprotect cases here
> (though perhaps you intend to later, iteratively?)
>
Yes.

> > +{
> > +     int ret;
> > +     int prot;
> > +     unsigned long size;
>
> I think 'size' is a bit confusing as it refers to the size ascertained from
> the get_vma_size() later on. Perhaps call it actual_size?
>
To help  readability, I will change size to vma_size, which
corresponds to the return of get_vma_size().

> Maybe worth adding:
>
>         void *ptr = self->ptr;
>         unsigned long size = self->size;
>         unsigned long page_size = self->page_size;
>
> For convenience?
>
> This is totally optional though, maybe more effort than it's worth.
>
I don't think it is worth it.

> > +
> > +     /* Seal both VMAs in one mseal call. */
> > +     ret = sys_mseal(self->ptr, self->size, 0);
> > +     ASSERT_EQ(ret, 0);
> > +
> > +     /* Verify mprotect is rejected on the first VMA. */
> > +     ret = sys_mprotect(self->ptr, self->page_size * 2,
> > +             PROT_READ | PROT_EXEC);
>
> Why PROT_READ | PROT_EXEC? Is this arbitrary? To be thorough shouldn't you
> loop through all of the prot flags that you think should fail? And perhaps
> check what happens if you mprotect() the VMA to set it to what it already
> is?
>
[resp 6]
Not arbitrary. As commented in FIXTURE_SETUP, the first VMA is
PROT_READ, and the second VMA is PROT_READ | PROT_WRITE.   The test
wants to use mprotect to change the proc bit to a different set.

For your question about mprotect() the sealed vma to what it already
is (doesn't change any attribute), I don't want to solidify the
current behavior (which is to block). It should be successful instead.
With in-loop check and partial update behavior,  there is no reason to
block it, i.e. no security benefit. I already plan to address this.

> > +     EXPECT_GT(0, ret);
>
> Please convert to EXPECT_NE(ret, 0)
>
please see [resp 4]

> > +     EXPECT_EQ(EPERM, errno);
>
> Please invert to EXPECT_EQ(errno, EPERM)
>
please see [resp 3]

> > +
> > +     /* Verify mprotect is rejected on the second VMA. */
> > +     ret = sys_mprotect(self->ptr, self->page_size * 2,
> > +             PROT_READ | PROT_EXEC);
>
> Hang on, isn't this just a repeat of the first test? Shouldn't this be:
>
>         ret = sys_mprotect(&self->ptr[self->page_size * 2],
>                 self->page_size * 2, PROT_READ | PROT_EXEC);
>
[resp 7] Not the same, this test has two VMA, and  the mseal() covers
both VMAs in one call. In the first test, i.e. basic, mprotect_basic,
mseal is called on single VMA.

> > +     EXPECT_GT(0, ret);
> > +     EXPECT_EQ(EPERM, errno);
>
> Same comment as above re: ordering.
>
please see [resp 3]

> > +
> > +     /* Attempt of mprotect two VMAs at the same call is blocked */
> > +     ret = sys_mprotect(self->ptr, self->size,
> > +             PROT_READ | PROT_EXEC);
>
> Same comment about iterating through prot flags that should fail as above.
>
please see [resp 7]

> > +     EXPECT_GT(0, ret);
> > +     EXPECT_EQ(EPERM, errno);
>
> Same comment as above re: ordering.
>
please see [resp 3]

> > +
> > +     /* Verify both VMAs aren't changed. */
> > +     size = get_vma_size(self->ptr, &prot);
> > +     EXPECT_EQ(self->page_size * 2, size);
> > +     EXPECT_EQ(PROT_READ, prot);
>
> Same comment as above re: ordering.
>
please see [resp 3]

> > +
> > +     size = get_vma_size(self->ptr + self->page_size * 2, &prot);
> > +     EXPECT_EQ(self->page_size * 2, size);
> > +     EXPECT_EQ(PROT_READ | PROT_WRITE, prot);
>
> Same comment as above re: ordering.
>
please see [resp 3]

> > +}
> > +
> > +TEST_HARNESS_MAIN
> > diff --git a/tools/testing/selftests/mm/memorysealing.h b/tools/testing/selftests/mm/memorysealing.h
> > new file mode 100644
> > index 000000000000..aee6e6700028
> > --- /dev/null
> > +++ b/tools/testing/selftests/mm/memorysealing.h
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <syscall.h>
> > +
> > +/*
> > + * Define sys_xyx to call syscall directly.
> > + * This is needed because we want to avoid calling glibc and
> > + * test syscall directly.
>
> I do wonder if this is worth it, but I mean, I guess it's not too egregious
> if it's wrapped like this. Same comment for other wrappers.
>
The self-test should directly test the kernel syscall without any
intermediate layer. selftest is not a libc test. It is practical too,
glibc's behavior can change across versions which result inconsistent,
and Padro Falcato reported one of this and fixed in [3]

> > + * The only exception is mmap, which _NR_mmap2 is not defined for
> > + * some ARM architecture.
> > + */
> > +static inline int sys_mseal(void *start, size_t len, unsigned long flags)
> > +{
> > +     int sret;
> > +
> > +     errno = 0;
>
> Why are we setting this to 0? Same comment for all sys_XXX() wrappers.
>
The test is only interested in the latest error associated with the
failed syscall, and I'm not sure the kernel clears the errno for each
system call. Note, there is no glibc layer here.

> > +     sret = syscall(__NR_mseal, start, len, flags);
> > +     return sret;
>
> Nit, but this seems a bit redundant, why not just drop the sret assignment,
> and simply:
>
>         return syscall(__NR_mseal, start, len, flags);
>
> Same comment for all sys_XXX() wrappers.
>
[resp 6] I think this is a different taste in coding style. For the
test, I prefer to have local variables so it is easy to show them in
the debugger, when needed.

> > +}
> > +
> > +static inline int sys_mprotect(void *ptr, size_t size, unsigned long prot)
> > +{
> > +     int sret;
> > +
> > +     errno = 0;
> > +     sret = syscall(__NR_mprotect, ptr, size, prot);
> > +     return sret;
> > +}
> > +
>
> The wrappers below don't seem to be used yet, could we delay putting them
> in until they are actually used?
>
Sure, I will remove sys_mprotect_pkey for this patch.

> > +static inline int sys_mprotect_pkey(void *ptr, size_t size,
> > +     unsigned long orig_prot, unsigned long pkey)
> > +{
> > +     int sret;
> > +
> > +     errno = 0;
> > +     sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey);
> > +     return sret;
> > +}
> > +
> > +static inline int sys_munmap(void *ptr, size_t size)
> > +{
> > +     int sret;
> > +
> > +     errno = 0;
> > +     sret = syscall(__NR_munmap, ptr, size);
> > +     return sret;
> > +}
> > +
> > +static inline int sys_madvise(void *start, size_t len, int types)
> > +{
> > +     int sret;
> > +
> > +     errno = 0;
> > +     sret = syscall(__NR_madvise, start, len, types);
> > +     return sret;
> > +}
> > +
> > +static inline void *sys_mremap(void *addr, size_t old_len, size_t new_len,
> > +     unsigned long flags, void *new_addr)
> > +{
> > +     void *sret;
> > +
> > +     errno = 0;
> > +     sret = (void *) syscall(__NR_mremap, addr, old_len, new_len, flags, new_addr);
> > +     return sret;
> > +}
> > +
> > +/*
> > + * Parsing /proc/self/maps to get VMA's size and prot bit.
> > + */
> > +static unsigned long get_vma_size(void *addr, int *prot)
> > +{
> > +     FILE *maps;
> > +     char line[256];
> > +     int size = 0;
> > +     uintptr_t  addr_start, addr_end;
>
> Why not add a:
>
>         uintptr_t addr_val = (uintptr_t)addr;
>
> To avoid having to constantly cast below?
>
sure.

> > +     char protstr[5];
> > +     *prot = 0;
>
> Nit but not sure there's much point in setting this to 0 even in failure
> cases?
>
> With an error code return you can avoid having to do this.
>
[resp 8]
I don't think the error code return is worth the effort, it currently
returns 0 size in case of error or not-found. For easy debug parsing
errors, I can add logs.  Notes, the parsing should always be
successful.

> > +
> > +     maps = fopen("/proc/self/maps", "r");
> > +     if (!maps)
> > +             return 0;
>
> Might it be better to return an error code or something so we can identify
> this case? I guess any sensible call will fail on a 0 but you're sort of
> just implicitly hoping for this.
>
[resp 9] we could add logging in case of error.

> > +
> > +     while (fgets(line, sizeof(line), maps)) {
> > +             if (sscanf(line, "%lx-%lx %4s", &addr_start, &addr_end, protstr) == 3) {
> > +                     if (addr_start == (uintptr_t) addr) {
>
> This is a Yoda comparison again, let's put what we're comparing first, and
> the thing we're comparing to afterwards, e.g.
>
>         if ((uintptr_t)addr == addr_start) ...
>
addr is the expected value from input and  addr_start changes after
parsing each line. The expected value (addr) is in second position,
why is this yoda syntax ? IMO, introducing the yoda syntax concept for
"=="  is more confusing than useful, A==B is as clear as B==A.

> Also I don't think a space after the (type) cast is necesary.
>
sure

> > +                             size = addr_end - addr_start;
> > +                             if (protstr[0] == 'r')
> > +                                     *prot |= PROT_READ;
> > +                             if (protstr[1] == 'w')
> > +                                     *prot |= PROT_WRITE;
> > +                             if (protstr[2] == 'x')
> > +                                     *prot |= PROT_EXEC;
> > +                             break;
> > +                     }
> > +             }
> > +     }
>
> On the impl: This would be neater if you used guard clauses:
>
>         while (fgets(line, sizeof(line), maps)) {
>                 if (sscanf(line, "%lx-%lx %4s", &addr_start, &addr_end, protstr) != 3)
>                         continue;
>
>                 if (addr_val != addr_start)
>                         continue;
>
>                 size = addr_end - addr_start;
>                 if (protstr[0] == 'r')
>                         *prot |= PROT_READ;
>                 if (protstr[1] == 'w')
>                         *prot |= PROT_WRITE;
>                 if (protstr[2] == 'x')
>                         *prot |= PROT_EXEC;
>                 break;
>         }
>
> This is a lot more readable?
>
sure.

> On doing this at all - it feels a bit crazy to check this, but I guess it
> might be the only way to assert things about merge/split.
>
> I'm not sure it's _really useful_ to do that though - merges/splits are
> internal implementation details in effect (modulo mremap single VMA
> requirements), and mseal is an external interface, really the check should
> be around 'can forbidden operations be performed on sealed mappings'.
>
This is related to resp [5].
This test not only verifies the size, but also verifies the prot bits.
The argument that VMA is an internal behavior is questionable, the
/proc/pid/maps is user-facing and applications can read it and parse
it.

> However, if you do really want to do this, I think it would be much better
> to use the new ioctl interface for this [0] which should avoid having to do
> unpleasant text wrangling :)
>
> [0]:https://lore.kernel.org/all/20240627170900.1672542-1-andrii@xxxxxxxxxx/
>
I could try this. This is a new feature though, I do think this
selftest should be self-standing and has as minimal dependency as
possible.  Is there harm of using /proc/pid/maps ? parsing
/proc/pid/maps already used by selftest in lots of places (see below),
I would prefer testing to use well-established patterns than relying
on a newly developed feature.

filesystems/overlayfs/dev_in_maps.c
mm/mlock2-tests.c
mm/mremap_test.c
mm/virtual_address_range.c
mm/split_huge_page_test.c
filesystems/overlayfs/dev_in_maps.c
exec/load_address.c
bpf/trace_helpers.c

>
> > +     fclose(maps);
> > +     return size;
>
> If we don't find the line we don't seem to be indicating this, again worth
> being able to pass back an error?
>
It can be normal that we don't find a line, i.e. tests want to verify
that a VMA does not exist at address, return 0 size is correct in this
case. If a parsing error ever happened, the test would have failed at
FIXTURE_SETUP to begin with.

> > +}
> > +
> > +static inline bool mseal_supported(void)
> > +{
> > +     int ret;
> > +     void *ptr;
> > +     unsigned long page_size = getpagesize();
> > +
> > +     ptr = mmap(NULL, page_size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > +     if (ptr == MAP_FAILED)
> > +             return false;
> > +
> > +     ret = sys_mseal(ptr, page_size, 0);
> > +     if (ret < 0)
> > +             return false;
>
> This is kind of cute, I mean it speaks to the need I think for us to be
> able to expose 'is X available?' programmatically. But I digress.
>
> I guess this is ok, as this is something that should never fail.
>
You might recall that mseal is not available on the 32 bits kernel,
automation might run the mseal_test on a 32 bit build.
mseal_supported() detects this so tests can be skipped.

> But we should definitely clean up after ourselves here whether this
> succeeds or fails.
>
please see resp [1].

Thanks!
-Jeff
[1] https://lore.kernel.org/all/CAKbZUD31EK2ah=vWJ46y4nL9OygzAa6ZxPnPHmWYO-sop5t+5Q@xxxxxxxxxxxxxx/
[2] https://lore.kernel.org/all/ko3pjllsgufbz33hqvwdpdsyxvgrgzqiodxexnpcng3mssffeh@dfgfkqlg46xa/
[3] https://lore.kernel.org/all/20240807153544.2754247-1-jeffxu@xxxxxxxxxxxx/







> > +
> > +     return true;
> > +}
> > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > index ad17005521a8..8dd20341de3d 100644
> > --- a/tools/testing/selftests/mm/mseal_test.c
> > +++ b/tools/testing/selftests/mm/mseal_test.c
> > @@ -517,30 +517,6 @@ static void test_seal_twice(void)
> >       REPORT_TEST_PASS();
> >  }
> >
> > -static void test_seal_mprotect(bool seal)
> > -{
> > -     void *ptr;
> > -     unsigned long page_size = getpagesize();
> > -     unsigned long size = 4 * page_size;
> > -     int ret;
> > -
> > -     setup_single_address(size, &ptr);
> > -     FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > -
> > -     if (seal) {
> > -             ret = seal_single_address(ptr, size);
> > -             FAIL_TEST_IF_FALSE(!ret);
> > -     }
> > -
> > -     ret = sys_mprotect(ptr, size, PROT_READ | PROT_WRITE);
> > -     if (seal)
> > -             FAIL_TEST_IF_FALSE(ret < 0);
> > -     else
> > -             FAIL_TEST_IF_FALSE(!ret);
> > -
> > -     REPORT_TEST_PASS();
> > -}
> > -
> >  static void test_seal_start_mprotect(bool seal)
> >  {
> >       void *ptr;
> > @@ -658,41 +634,6 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal)
> >       REPORT_TEST_PASS();
> >  }
> >
> > -static void test_seal_mprotect_two_vma(bool seal)
> > -{
> > -     void *ptr;
> > -     unsigned long page_size = getpagesize();
> > -     unsigned long size = 4 * page_size;
> > -     int ret;
> > -
> > -     setup_single_address(size, &ptr);
> > -     FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > -
> > -     /* use mprotect to split */
> > -     ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
> > -     FAIL_TEST_IF_FALSE(!ret);
> > -
> > -     if (seal) {
> > -             ret = seal_single_address(ptr, page_size * 4);
> > -             FAIL_TEST_IF_FALSE(!ret);
> > -     }
> > -
> > -     ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
> > -     if (seal)
> > -             FAIL_TEST_IF_FALSE(ret < 0);
> > -     else
> > -             FAIL_TEST_IF_FALSE(!ret);
> > -
> > -     ret = sys_mprotect(ptr + page_size * 2, page_size * 2,
> > -                     PROT_READ | PROT_WRITE);
> > -     if (seal)
> > -             FAIL_TEST_IF_FALSE(ret < 0);
> > -     else
> > -             FAIL_TEST_IF_FALSE(!ret);
> > -
> > -     REPORT_TEST_PASS();
> > -}
> > -
> >  static void test_seal_mprotect_two_vma_with_split(bool seal)
> >  {
> >       void *ptr;
> > @@ -1876,7 +1817,7 @@ int main(void)
> >       if (!pkey_supported())
> >               ksft_print_msg("PKEY not supported\n");
> >
> > -     ksft_set_plan(88);
> > +     ksft_set_plan(84);
> >
> >       test_seal_addseal();
> >       test_seal_unmapped_start();
> > @@ -1889,9 +1830,6 @@ int main(void)
> >       test_seal_zero_length();
> >       test_seal_twice();
> >
> > -     test_seal_mprotect(false);
> > -     test_seal_mprotect(true);
> > -
> >       test_seal_start_mprotect(false);
> >       test_seal_start_mprotect(true);
> >
> > @@ -1904,9 +1842,6 @@ int main(void)
> >       test_seal_mprotect_unalign_len_variant_2(false);
> >       test_seal_mprotect_unalign_len_variant_2(true);
> >
> > -     test_seal_mprotect_two_vma(false);
> > -     test_seal_mprotect_two_vma(true);
> > -
> >       test_seal_mprotect_two_vma_with_split(false);
> >       test_seal_mprotect_two_vma_with_split(true);
> >
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
> >