diff mbox series

[v1] selftest mm/mseal: fix test_seal_mremap_move_dontunmap_anyaddr

Message ID 20240807153544.2754247-1-jeffxu@chromium.org (mailing list archive)
State New
Headers show
Series [v1] selftest mm/mseal: fix test_seal_mremap_move_dontunmap_anyaddr | expand

Commit Message

Jeff Xu Aug. 7, 2024, 3:35 p.m. UTC 
From: Jeff Xu <jeffxu@chromium.org>

the syscall remap accepts following:

sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
		(void*) 0xdead0000)

when the src is sealed, the call will fail with when error code:
EPERM

Also remove the dependency on glibc for mremap(), the previous
version of test pass on certain version of glibc and fail in another.
By calling syscall directly, we test and verify remap function
in kernel directly.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
Reported-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 tools/testing/selftests/mm/mseal_test.c | 44 ++++++++++++++-----------
 1 file changed, 25 insertions(+), 19 deletions(-)

Comments

Pedro Falcato Aug. 7, 2024, 4:38 p.m. UTC | #1 
On Wed, Aug 7, 2024 at 4:35 PM <jeffxu@chromium.org> wrote:
<snip>
>         /* shrink from 4 pages to 2 pages. */
> -       ret2 = mremap(ptr, size, 2 * page_size, 0, 0);
> +       ret2 = sys_mremap(ptr, size, 2 * page_size, 0, 0);
>         if (seal) {
> -               FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> +               FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);

MAP_FAILED is already void *

<snip>
> @@ -1449,18 +1457,16 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal)
>         }
>
>         /*
> -        * The 0xdeaddead should not have effect on dest addr
> +        * The 0xdead0000 should not have effect on dest addr
>          * when MREMAP_DONTUNMAP is set.
>          */
> -       ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
> -                       0xdeaddead);
> +       ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
> +                       (void *) 0xdead0000);

You still didn't explain why this test is actually needed. Why are you
testing MREMAP_DONTUNMAP's hint system?
This has nothing to do with mseal, you already test the
MREMAP_DONTUNMAP and MREMAP_FIXED paths in other tests.
You also don't know if 0xdead0000 is a valid page (hexagon for
instance seems to support 256KiB and 1MiB pages, so does ppc32, and
this is not something that should be hardcoded).

I'm not a fan of just throwing random flags for tests, it should be
somewhat logical.
Jeff Xu Aug. 7, 2024, 6:02 p.m. UTC | #2 
On Wed, Aug 7, 2024 at 9:38 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 4:35 PM <jeffxu@chromium.org> wrote:
> <snip>
> >         /* shrink from 4 pages to 2 pages. */
> > -       ret2 = mremap(ptr, size, 2 * page_size, 0, 0);
> > +       ret2 = sys_mremap(ptr, size, 2 * page_size, 0, 0);
> >         if (seal) {
> > -               FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > +               FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
>
> MAP_FAILED is already void *
>
> <snip>
> > @@ -1449,18 +1457,16 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal)
> >         }
> >
> >         /*
> > -        * The 0xdeaddead should not have effect on dest addr
> > +        * The 0xdead0000 should not have effect on dest addr
> >          * when MREMAP_DONTUNMAP is set.
> >          */
> > -       ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
> > -                       0xdeaddead);
> > +       ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
> > +                       (void *) 0xdead0000);
>
> You still didn't explain why this test is actually needed. Why are you
> testing MREMAP_DONTUNMAP's hint system?

I responded in my previous email. The test is to make sure when
sealing is applied, the call fails with correct error code. I will
update the comment in v2 to clarify that.

> This has nothing to do with mseal, you already test the
> MREMAP_DONTUNMAP and MREMAP_FIXED paths in other tests.
The remap code path is quite tricky, with many flags directing the call flow.
The difference might not be that obvious:

test_seal_mremap_move_dontunmap use 0 as new_addr, 0 indicates
allocating a new memory.
test_seal_mremap_move_dontunmap_anyaddr uses any arbitrary address as
a new address.

> You also don't know if 0xdead0000 is a valid page (hexagon for
> instance seems to support 256KiB and 1MiB pages, so does ppc32, and
> this is not something that should be hardcoded).
>
usually hardcode value is not good practice, but the point of this
test is to show
mremap can really relocate the mapping to an arbitrary address.

Do you have any suggestions here ? I can think of two options to choose from:

1> use 0xd0000000
2> allocate a memory then free it, reuse the ptr.

Thanks
-Jeff
Jeff Xu Aug. 7, 2024, 9:06 p.m. UTC | #3 
On Wed, Aug 7, 2024 at 11:03 AM Jeff Xu <jeffxu@google.com> wrote:
>
> On Wed, Aug 7, 2024 at 9:38 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Wed, Aug 7, 2024 at 4:35 PM <jeffxu@chromium.org> wrote:
> > <snip>
> > >         /* shrink from 4 pages to 2 pages. */
> > > -       ret2 = mremap(ptr, size, 2 * page_size, 0, 0);
> > > +       ret2 = sys_mremap(ptr, size, 2 * page_size, 0, 0);
> > >         if (seal) {
> > > -               FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
> > > +               FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
> >
> > MAP_FAILED is already void *
> >
> > <snip>
> > > @@ -1449,18 +1457,16 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal)
> > >         }
> > >
> > >         /*
> > > -        * The 0xdeaddead should not have effect on dest addr
> > > +        * The 0xdead0000 should not have effect on dest addr
> > >          * when MREMAP_DONTUNMAP is set.
> > >          */
> > > -       ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
> > > -                       0xdeaddead);
> > > +       ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
> > > +                       (void *) 0xdead0000);
> >
> > You still didn't explain why this test is actually needed. Why are you
> > testing MREMAP_DONTUNMAP's hint system?
>
> I responded in my previous email. The test is to make sure when
> sealing is applied, the call fails with correct error code. I will
> update the comment in v2 to clarify that.
>
> > This has nothing to do with mseal, you already test the
> > MREMAP_DONTUNMAP and MREMAP_FIXED paths in other tests.
> The remap code path is quite tricky, with many flags directing the call flow.
> The difference might not be that obvious:
>
> test_seal_mremap_move_dontunmap use 0 as new_addr, 0 indicates
> allocating a new memory.
> test_seal_mremap_move_dontunmap_anyaddr uses any arbitrary address as
> a new address.
>
> > You also don't know if 0xdead0000 is a valid page (hexagon for
> > instance seems to support 256KiB and 1MiB pages, so does ppc32, and
> > this is not something that should be hardcoded).
> >
> usually hardcode value is not good practice, but the point of this
> test is to show
> mremap can really relocate the mapping to an arbitrary address.
>
> Do you have any suggestions here ? I can think of two options to choose from:
>
> 1> use 0xd0000000
> 2> allocate a memory then free it, reuse the ptr.
>
I will send out V2 that addresses those comments above.
Thanks
Pedro Falcato Aug. 7, 2024, 9:20 p.m. UTC | #4 
On Wed, Aug 7, 2024 at 7:03 PM Jeff Xu <jeffxu@google.com> wrote:
<snip>
>
> test_seal_mremap_move_dontunmap use 0 as new_addr, 0 indicates
> allocating a new memory.
> test_seal_mremap_move_dontunmap_anyaddr uses any arbitrary address as
> a new address.

No, MREMAP_DONTUNMAP uses the address you pass as a hint, aka you're
just testing get_unmapped_area, not any mseal capability.
There's no forced moving here.

>
> > You also don't know if 0xdead0000 is a valid page (hexagon for
> > instance seems to support 256KiB and 1MiB pages, so does ppc32, and
> > this is not something that should be hardcoded).
> >
> usually hardcode value is not good practice, but the point of this
> test is to show
> mremap can really relocate the mapping to an arbitrary address.

That's what test_seal_mremap_move_dontunmap does, no?

>
> Do you have any suggestions here ? I can think of two options to choose from:
>
> 1> use 0xd0000000
> 2> allocate a memory then free it, reuse the ptr.

Personally I'd prefer 2, if you really want to keep the test. It's
also a strategy used elsewhere (e.g mremap_dontunmap.c).

FWIW I don't have the mental strength to bikeshed over this any more,
so please do what you think is best!
Jeff Xu Aug. 7, 2024, 9:34 p.m. UTC | #5 
On Wed, Aug 7, 2024 at 2:20 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 7:03 PM Jeff Xu <jeffxu@google.com> wrote:
> > Do you have any suggestions here ? I can think of two options to choose from:
> >
> > 1> use 0xd0000000
> > 2> allocate a memory then free it, reuse the ptr.
>
> Personally I'd prefer 2, if you really want to keep the test. It's
> also a strategy used elsewhere (e.g mremap_dontunmap.c).
>
V2 is sent with 2.
Thanks for reporting this.

-Jeff
diff mbox series

Patch

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index a818f010de47..45d299fd74ed 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -110,6 +110,16 @@  static int sys_madvise(void *start, size_t len, int types)
 	return sret;
 }
 
+static 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;
+}
+
 static int sys_pkey_alloc(unsigned long flags, unsigned long init_val)
 {
 	int ret = syscall(__NR_pkey_alloc, flags, init_val);
@@ -1115,12 +1125,12 @@  static void test_seal_mremap_shrink(bool seal)
 	}
 
 	/* shrink from 4 pages to 2 pages. */
-	ret2 = mremap(ptr, size, 2 * page_size, 0, 0);
+	ret2 = sys_mremap(ptr, size, 2 * page_size, 0, 0);
 	if (seal) {
-		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
+		FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
 	} else {
-		FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
+		FAIL_TEST_IF_FALSE(ret2 != (void *) MAP_FAILED);
 
 	}
 
@@ -1147,7 +1157,7 @@  static void test_seal_mremap_expand(bool seal)
 	}
 
 	/* expand from 2 page to 4 pages. */
-	ret2 = mremap(ptr, 2 * page_size, 4 * page_size, 0, 0);
+	ret2 = sys_mremap(ptr, 2 * page_size, 4 * page_size, 0, 0);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
@@ -1180,7 +1190,7 @@  static void test_seal_mremap_move(bool seal)
 	}
 
 	/* move from ptr to fixed address. */
-	ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_FIXED, newPtr);
+	ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_FIXED, newPtr);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
@@ -1299,7 +1309,7 @@  static void test_seal_mremap_shrink_fixed(bool seal)
 	}
 
 	/* mremap to move and shrink to fixed address */
-	ret2 = mremap(ptr, size, 2 * page_size, MREMAP_MAYMOVE | MREMAP_FIXED,
+	ret2 = sys_mremap(ptr, size, 2 * page_size, MREMAP_MAYMOVE | MREMAP_FIXED,
 			newAddr);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
@@ -1330,7 +1340,7 @@  static void test_seal_mremap_expand_fixed(bool seal)
 	}
 
 	/* mremap to move and expand to fixed address */
-	ret2 = mremap(ptr, page_size, size, MREMAP_MAYMOVE | MREMAP_FIXED,
+	ret2 = sys_mremap(ptr, page_size, size, MREMAP_MAYMOVE | MREMAP_FIXED,
 			newAddr);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
@@ -1361,7 +1371,7 @@  static void test_seal_mremap_move_fixed(bool seal)
 	}
 
 	/* mremap to move to fixed address */
-	ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_FIXED, newAddr);
+	ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_FIXED, newAddr);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
@@ -1390,14 +1400,13 @@  static void test_seal_mremap_move_fixed_zero(bool seal)
 	/*
 	 * MREMAP_FIXED can move the mapping to zero address
 	 */
-	ret2 = mremap(ptr, size, 2 * page_size, MREMAP_MAYMOVE | MREMAP_FIXED,
+	ret2 = sys_mremap(ptr, size, 2 * page_size, MREMAP_MAYMOVE | MREMAP_FIXED,
 			0);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
 	} else {
 		FAIL_TEST_IF_FALSE(ret2 == 0);
-
 	}
 
 	REPORT_TEST_PASS();
@@ -1420,13 +1429,12 @@  static void test_seal_mremap_move_dontunmap(bool seal)
 	}
 
 	/* mremap to move, and don't unmap src addr. */
-	ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP, 0);
+	ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP, 0);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
 	} else {
 		FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
-
 	}
 
 	REPORT_TEST_PASS();
@@ -1449,18 +1457,16 @@  static void test_seal_mremap_move_dontunmap_anyaddr(bool seal)
 	}
 
 	/*
-	 * The 0xdeaddead should not have effect on dest addr
+	 * The 0xdead0000 should not have effect on dest addr
 	 * when MREMAP_DONTUNMAP is set.
 	 */
-	ret2 = mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
-			0xdeaddead);
+	ret2 = sys_mremap(ptr, size, size, MREMAP_MAYMOVE | MREMAP_DONTUNMAP,
+			(void *) 0xdead0000);
 	if (seal) {
-		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
+		FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
 	} else {
-		FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
-		FAIL_TEST_IF_FALSE((long)ret2 != 0xdeaddead);
-
+		FAIL_TEST_IF_FALSE(ret2 == (void *) 0xdead0000);
 	}
 
 	REPORT_TEST_PASS();