diff mbox series

[v3,7/7] selftests/mm: add more mseal traversal tests

Message ID 20240817-mseal-depessimize-v3-7-d8d2e037df30@gmail.com (mailing list archive)
State New
Headers show
Series mm: Optimize mseal checks | expand

Commit Message

Pedro Falcato Aug. 17, 2024, 12:18 a.m. UTC
Add more mseal traversal tests across VMAs, where we could possibly
screw up sealing checks. These test more across-vma traversal for
mprotect, munmap and madvise. Particularly, we test for the case where a
regular VMA is followed by a sealed VMA.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 tools/testing/selftests/mm/mseal_test.c | 111 +++++++++++++++++++++++++++++++-
 1 file changed, 110 insertions(+), 1 deletion(-)

Comments

Pedro Falcato Aug. 18, 2024, 6:36 a.m. UTC | #1
On Sat, Aug 17, 2024 at 1:18 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
>         REPORT_TEST_PASS();
>  }
>
> +static void test_seal_munmap_partial_across_vmas(bool seal)
> +{
> +       void *ptr;
> +       unsigned long page_size = getpagesize();
> +       unsigned long size = 2 * page_size;
> +       int ret;
> +       int prot;
> +
> +       /*
> +        * Check if a partial mseal (that results in two vmas) works correctly.
> +        * It might unmap the first, but it'll never unmap the second (msealed) vma.
> +        */

Bah, obviously this comment isn't true, munmap is never partial.
I'll change this locally for v4 if there ends up being one.
Liam R. Howlett Aug. 20, 2024, 3:45 p.m. UTC | #2
* Pedro Falcato <pedro.falcato@gmail.com> [240818 02:36]:
> On Sat, Aug 17, 2024 at 1:18 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
> >         REPORT_TEST_PASS();
> >  }
> >
> > +static void test_seal_munmap_partial_across_vmas(bool seal)
> > +{
> > +       void *ptr;
> > +       unsigned long page_size = getpagesize();
> > +       unsigned long size = 2 * page_size;
> > +       int ret;
> > +       int prot;
> > +
> > +       /*
> > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > +        * It might unmap the first, but it'll never unmap the second (msealed) vma.
> > +        */
> 
> Bah, obviously this comment isn't true, munmap is never partial.
> I'll change this locally for v4 if there ends up being one.

Besides this comment, the patch looks good.

Reviewed-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
Lorenzo Stoakes Aug. 21, 2024, 8:47 a.m. UTC | #3
On Sat, Aug 17, 2024 at 01:18:34AM GMT, Pedro Falcato wrote:
> Add more mseal traversal tests across VMAs, where we could possibly
> screw up sealing checks. These test more across-vma traversal for
> mprotect, munmap and madvise. Particularly, we test for the case where a
> regular VMA is followed by a sealed VMA.
>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>

Other than the comment, LGTM.

Thanks for this series!

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  tools/testing/selftests/mm/mseal_test.c | 111 +++++++++++++++++++++++++++++++-
>  1 file changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index 259bef4945e9..0d4d40fb0f88 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -766,6 +766,42 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
>  	REPORT_TEST_PASS();
>  }
>
> +static void test_seal_mprotect_partial_mprotect_tail(bool seal)
> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = 2 * page_size;
> +	int ret;
> +	int prot;
> +
> +	/*
> +	 * Check if a partial mseal (that results in two vmas) works correctly.
> +	 * It might mprotect the first, but it'll never touch the second (msealed) vma.
> +	 */
> +
> +	setup_single_address(size, &ptr);
> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +	if (seal) {
> +		ret = sys_mseal(ptr + page_size, size);
> +		FAIL_TEST_IF_FALSE(!ret);
> +	}
> +
> +	ret = sys_mprotect(ptr, size, PROT_EXEC);
> +	if (seal)
> +		FAIL_TEST_IF_FALSE(ret < 0);
> +	else
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +	if (seal) {
> +		FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);
> +	}
> +
> +	REPORT_TEST_PASS();
> +}
> +
> +
>  static void test_seal_mprotect_two_vma_with_gap(bool seal)
>  {
>  	void *ptr;
> @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
>  	REPORT_TEST_PASS();
>  }
>
> +static void test_seal_munmap_partial_across_vmas(bool seal)
> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = 2 * page_size;
> +	int ret;
> +	int prot;
> +
> +	/*
> +	 * Check if a partial mseal (that results in two vmas) works correctly.
> +	 * It might unmap the first, but it'll never unmap the second (msealed) vma.
> +	 */
> +
> +	setup_single_address(size, &ptr);
> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +	if (seal) {
> +		ret = sys_mseal(ptr + page_size, size);
> +		FAIL_TEST_IF_FALSE(!ret);
> +	}
> +
> +	ret = sys_munmap(ptr, size);
> +	if (seal)
> +		FAIL_TEST_IF_FALSE(ret < 0);
> +	else
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +	if (seal) {
> +		FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> +		FAIL_TEST_IF_FALSE(prot == 0x4);
> +	}
> +
> +	REPORT_TEST_PASS();
> +}
> +
>  static void test_munmap_start_freed(bool seal)
>  {
>  	void *ptr;
> @@ -1735,6 +1806,37 @@ static void test_seal_discard_ro_anon(bool seal)
>  	REPORT_TEST_PASS();
>  }
>
> +static void test_seal_discard_across_vmas(bool seal)
> +{
> +	void *ptr;
> +	unsigned long page_size = getpagesize();
> +	unsigned long size = 2 * page_size;
> +	int ret;
> +
> +	setup_single_address(size, &ptr);
> +	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +	if (seal) {
> +		ret = seal_single_address(ptr + page_size, page_size);
> +		FAIL_TEST_IF_FALSE(!ret);
> +	}
> +
> +	ret = sys_madvise(ptr, size, MADV_DONTNEED);
> +	if (seal)
> +		FAIL_TEST_IF_FALSE(ret < 0);
> +	else
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +	ret = sys_munmap(ptr, size);
> +	if (seal)
> +		FAIL_TEST_IF_FALSE(ret < 0);
> +	else
> +		FAIL_TEST_IF_FALSE(!ret);
> +
> +	REPORT_TEST_PASS();
> +}
> +
> +
>  static void test_seal_madvise_nodiscard(bool seal)
>  {
>  	void *ptr;
> @@ -1779,7 +1881,7 @@ int main(int argc, char **argv)
>  	if (!pkey_supported())
>  		ksft_print_msg("PKEY not supported\n");
>
> -	ksft_set_plan(82);
> +	ksft_set_plan(88);
>
>  	test_seal_addseal();
>  	test_seal_unmapped_start();
> @@ -1825,12 +1927,17 @@ int main(int argc, char **argv)
>  	test_seal_mprotect_split(false);
>  	test_seal_mprotect_split(true);
>
> +	test_seal_mprotect_partial_mprotect_tail(false);
> +	test_seal_mprotect_partial_mprotect_tail(true);
> +
>  	test_seal_munmap(false);
>  	test_seal_munmap(true);
>  	test_seal_munmap_two_vma(false);
>  	test_seal_munmap_two_vma(true);
>  	test_seal_munmap_vma_with_gap(false);
>  	test_seal_munmap_vma_with_gap(true);
> +	test_seal_munmap_partial_across_vmas(false);
> +	test_seal_munmap_partial_across_vmas(true);
>
>  	test_munmap_start_freed(false);
>  	test_munmap_start_freed(true);
> @@ -1862,6 +1969,8 @@ int main(int argc, char **argv)
>  	test_seal_madvise_nodiscard(true);
>  	test_seal_discard_ro_anon(false);
>  	test_seal_discard_ro_anon(true);
> +	test_seal_discard_across_vmas(false);
> +	test_seal_discard_across_vmas(true);
>  	test_seal_discard_ro_anon_on_rw(false);
>  	test_seal_discard_ro_anon_on_rw(true);
>  	test_seal_discard_ro_anon_on_shared(false);
>
> --
> 2.46.0
>
Jeff Xu Aug. 21, 2024, 3:56 p.m. UTC | #4
Hi Pedro

On Fri, Aug 16, 2024 at 5:18 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> Add more mseal traversal tests across VMAs, where we could possibly
> screw up sealing checks. These test more across-vma traversal for
> mprotect, munmap and madvise. Particularly, we test for the case where a
> regular VMA is followed by a sealed VMA.
>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
>  tools/testing/selftests/mm/mseal_test.c | 111 +++++++++++++++++++++++++++++++-
>  1 file changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index 259bef4945e9..0d4d40fb0f88 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -766,6 +766,42 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
>         REPORT_TEST_PASS();
>  }
>
> +static void test_seal_mprotect_partial_mprotect_tail(bool seal)
> +{
> +       void *ptr;
> +       unsigned long page_size = getpagesize();
> +       unsigned long size = 2 * page_size;
> +       int ret;
> +       int prot;
> +
> +       /*
> +        * Check if a partial mseal (that results in two vmas) works correctly.
> +        * It might mprotect the first, but it'll never touch the second (msealed) vma.
> +        */
> +
> +       setup_single_address(size, &ptr);
> +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +       if (seal) {
> +               ret = sys_mseal(ptr + page_size, size);
you are allocating 2 pages , and I assume you are sealing the second
page, so the size should be page_size.
ret = sys_mseal(ptr + page_size, page_size);

> +               FAIL_TEST_IF_FALSE(!ret);
> +       }
> +
> +       ret = sys_mprotect(ptr, size, PROT_EXEC);
> +       if (seal)
> +               FAIL_TEST_IF_FALSE(ret < 0);
> +       else
> +               FAIL_TEST_IF_FALSE(!ret);
> +
> +       if (seal) {
> +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> +               FAIL_TEST_IF_FALSE(prot == 0x4);
To test partial mprotect, the test needs to add the check for the
first page to be changed, Also to avoid the merge,  a PROT_NONE page
can to be added in front.

> +       }
> +
> +       REPORT_TEST_PASS();
> +}
> +
> +
>  static void test_seal_mprotect_two_vma_with_gap(bool seal)
>  {
>         void *ptr;
> @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
>         REPORT_TEST_PASS();
>  }
>
> +static void test_seal_munmap_partial_across_vmas(bool seal)
> +{
> +       void *ptr;
> +       unsigned long page_size = getpagesize();
> +       unsigned long size = 2 * page_size;
> +       int ret;
> +       int prot;
> +
> +       /*
> +        * Check if a partial mseal (that results in two vmas) works correctly.
> +        * It might unmap the first, but it'll never unmap the second (msealed) vma.
> +        */
> +
> +       setup_single_address(size, &ptr);
> +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +       if (seal) {
> +               ret = sys_mseal(ptr + page_size, size);
ret = sys_mseal(ptr + page_size, page_size);

> +               FAIL_TEST_IF_FALSE(!ret);
> +       }
> +
> +       ret = sys_munmap(ptr, size);
> +       if (seal)
> +               FAIL_TEST_IF_FALSE(ret < 0);
> +       else
> +               FAIL_TEST_IF_FALSE(!ret);
> +
> +       if (seal) {
> +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> +               FAIL_TEST_IF_FALSE(prot == 0x4);
To test partial unmap, the test needs to add the check for the first
page to be freed, Also to avoid the merge,  a PROT_NONE page needs to
be in front.

The test_seal_munmap_partial_across_vmas  shows the behavior
difference with in-loop approach and out-loop approach. Previously,
both VMAs will not be freed, now the first VMA will be freed, and the
second VMA (sealed) won't.

This brings to the line you previously mentioned: [1] and I quote:
"munmap is atomic and always has been. It's required by POSIX."

So this is no longer true for the current series.  Linux doesn't need
to be POSIX compliant, from previous conversation on this topic with
Linus [2], so I'm open to that. If this is accepted by Linus, it would
be better to add comments on munmap code or tests, for future readers
(in case they are curious about reasoning. )

[1] https://lore.kernel.org/linux-mm/CAKbZUD3_3KN4fAyQsD+=p3PV8svAvVyS278umX40Ehsa+zkz3w@mail.gmail.com/
[2] https://lore.kernel.org/linux-mm/CAHk-=wgDv5vPx2xoxNQh+kbvLsskWubGGGK69cqF_i4FkM-GCw@mail.gmail.com/

> +       }
> +
> +       REPORT_TEST_PASS();
> +}
> +
>  static void test_munmap_start_freed(bool seal)
>  {
>         void *ptr;
> @@ -1735,6 +1806,37 @@ static void test_seal_discard_ro_anon(bool seal)
>         REPORT_TEST_PASS();
>  }
>
> +static void test_seal_discard_across_vmas(bool seal)
> +{
> +       void *ptr;
> +       unsigned long page_size = getpagesize();
> +       unsigned long size = 2 * page_size;
> +       int ret;
> +
> +       setup_single_address(size, &ptr);
> +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> +       if (seal) {
> +               ret = seal_single_address(ptr + page_size, page_size);
> +               FAIL_TEST_IF_FALSE(!ret);
> +       }
> +
> +       ret = sys_madvise(ptr, size, MADV_DONTNEED);
> +       if (seal)
> +               FAIL_TEST_IF_FALSE(ret < 0);
> +       else
> +               FAIL_TEST_IF_FALSE(!ret);
> +
> +       ret = sys_munmap(ptr, size);
> +       if (seal)
> +               FAIL_TEST_IF_FALSE(ret < 0);
> +       else
> +               FAIL_TEST_IF_FALSE(!ret);
> +
> +       REPORT_TEST_PASS();
> +}
> +
> +
>  static void test_seal_madvise_nodiscard(bool seal)
>  {
>         void *ptr;
> @@ -1779,7 +1881,7 @@ int main(int argc, char **argv)
>         if (!pkey_supported())
>                 ksft_print_msg("PKEY not supported\n");
>
> -       ksft_set_plan(82);
> +       ksft_set_plan(88);
>
>         test_seal_addseal();
>         test_seal_unmapped_start();
> @@ -1825,12 +1927,17 @@ int main(int argc, char **argv)
>         test_seal_mprotect_split(false);
>         test_seal_mprotect_split(true);
>
> +       test_seal_mprotect_partial_mprotect_tail(false);
> +       test_seal_mprotect_partial_mprotect_tail(true);
> +
>         test_seal_munmap(false);
>         test_seal_munmap(true);
>         test_seal_munmap_two_vma(false);
>         test_seal_munmap_two_vma(true);
>         test_seal_munmap_vma_with_gap(false);
>         test_seal_munmap_vma_with_gap(true);
> +       test_seal_munmap_partial_across_vmas(false);
> +       test_seal_munmap_partial_across_vmas(true);
>
>         test_munmap_start_freed(false);
>         test_munmap_start_freed(true);
> @@ -1862,6 +1969,8 @@ int main(int argc, char **argv)
>         test_seal_madvise_nodiscard(true);
>         test_seal_discard_ro_anon(false);
>         test_seal_discard_ro_anon(true);
> +       test_seal_discard_across_vmas(false);
> +       test_seal_discard_across_vmas(true);
>         test_seal_discard_ro_anon_on_rw(false);
>         test_seal_discard_ro_anon_on_rw(true);
>         test_seal_discard_ro_anon_on_shared(false);
>
> --
> 2.46.0
>
>
Pedro Falcato Aug. 21, 2024, 4:20 p.m. UTC | #5
On Wed, Aug 21, 2024 at 4:56 PM Jeff Xu <jeffxu@chromium.org> wrote:
>
> Hi Pedro
>
> On Fri, Aug 16, 2024 at 5:18 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > Add more mseal traversal tests across VMAs, where we could possibly
> > screw up sealing checks. These test more across-vma traversal for
> > mprotect, munmap and madvise. Particularly, we test for the case where a
> > regular VMA is followed by a sealed VMA.
> >
> > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > ---
> >  tools/testing/selftests/mm/mseal_test.c | 111 +++++++++++++++++++++++++++++++-
> >  1 file changed, 110 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > index 259bef4945e9..0d4d40fb0f88 100644
> > --- a/tools/testing/selftests/mm/mseal_test.c
> > +++ b/tools/testing/selftests/mm/mseal_test.c
> > @@ -766,6 +766,42 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
> >         REPORT_TEST_PASS();
> >  }
> >
> > +static void test_seal_mprotect_partial_mprotect_tail(bool seal)
> > +{
> > +       void *ptr;
> > +       unsigned long page_size = getpagesize();
> > +       unsigned long size = 2 * page_size;
> > +       int ret;
> > +       int prot;
> > +
> > +       /*
> > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > +        * It might mprotect the first, but it'll never touch the second (msealed) vma.
> > +        */
> > +
> > +       setup_single_address(size, &ptr);
> > +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > +
> > +       if (seal) {
> > +               ret = sys_mseal(ptr + page_size, size);
> you are allocating 2 pages , and I assume you are sealing the second
> page, so the size should be page_size.
> ret = sys_mseal(ptr + page_size, page_size);

Yes, good catch, it appears to be harmless but ofc down to straight luck.
I'll send a fixup for this and the other mistake down there.

>
> > +               FAIL_TEST_IF_FALSE(!ret);
> > +       }
> > +
> > +       ret = sys_mprotect(ptr, size, PROT_EXEC);
> > +       if (seal)
> > +               FAIL_TEST_IF_FALSE(ret < 0);
> > +       else
> > +               FAIL_TEST_IF_FALSE(!ret);
> > +
> > +       if (seal) {
> > +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> > +               FAIL_TEST_IF_FALSE(prot == 0x4);
> To test partial mprotect, the test needs to add the check for the
> first page to be changed, Also to avoid the merge,  a PROT_NONE page
> can to be added in front.

No, I'm leaving partial mprotect to be undefined. It doesn't make
sense to constraint ourselves, since POSIX wording is already loose.

>
> > +       }
> > +
> > +       REPORT_TEST_PASS();
> > +}
> > +
> > +
> >  static void test_seal_mprotect_two_vma_with_gap(bool seal)
> >  {
> >         void *ptr;
> > @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
> >         REPORT_TEST_PASS();
> >  }
> >
> > +static void test_seal_munmap_partial_across_vmas(bool seal)
> > +{
> > +       void *ptr;
> > +       unsigned long page_size = getpagesize();
> > +       unsigned long size = 2 * page_size;
> > +       int ret;
> > +       int prot;
> > +
> > +       /*
> > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > +        * It might unmap the first, but it'll never unmap the second (msealed) vma.
> > +        */
> > +
> > +       setup_single_address(size, &ptr);
> > +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > +
> > +       if (seal) {
> > +               ret = sys_mseal(ptr + page_size, size);
> ret = sys_mseal(ptr + page_size, page_size);
>
> > +               FAIL_TEST_IF_FALSE(!ret);
> > +       }
> > +
> > +       ret = sys_munmap(ptr, size);
> > +       if (seal)
> > +               FAIL_TEST_IF_FALSE(ret < 0);
> > +       else
> > +               FAIL_TEST_IF_FALSE(!ret);
> > +
> > +       if (seal) {
> > +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> > +               FAIL_TEST_IF_FALSE(prot == 0x4);
> To test partial unmap, the test needs to add the check for the first
> page to be freed, Also to avoid the merge,  a PROT_NONE page needs to
> be in front.

I'm not testing partial unmap. Partial unmap does not happen. I have
told you this before.

>
> The test_seal_munmap_partial_across_vmas  shows the behavior
> difference with in-loop approach and out-loop approach. Previously,
> both VMAs will not be freed, now the first VMA will be freed, and the
> second VMA (sealed) won't.
>
> This brings to the line you previously mentioned: [1] and I quote:
> "munmap is atomic and always has been. It's required by POSIX."

This is still true, the comment was a copy-and-paste mindslip. Please
read the email thread. It has been fixed up by Andrew.
Jeff Xu Aug. 21, 2024, 4:27 p.m. UTC | #6
On Wed, Aug 21, 2024 at 9:20 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Wed, Aug 21, 2024 at 4:56 PM Jeff Xu <jeffxu@chromium.org> wrote:
> >
> > Hi Pedro
> >
> > On Fri, Aug 16, 2024 at 5:18 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > >
> > > Add more mseal traversal tests across VMAs, where we could possibly
> > > screw up sealing checks. These test more across-vma traversal for
> > > mprotect, munmap and madvise. Particularly, we test for the case where a
> > > regular VMA is followed by a sealed VMA.
> > >
> > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > > ---
> > >  tools/testing/selftests/mm/mseal_test.c | 111 +++++++++++++++++++++++++++++++-
> > >  1 file changed, 110 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > > index 259bef4945e9..0d4d40fb0f88 100644
> > > --- a/tools/testing/selftests/mm/mseal_test.c
> > > +++ b/tools/testing/selftests/mm/mseal_test.c
> > > @@ -766,6 +766,42 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
> > >         REPORT_TEST_PASS();
> > >  }
> > >
> > > +static void test_seal_mprotect_partial_mprotect_tail(bool seal)
> > > +{
> > > +       void *ptr;
> > > +       unsigned long page_size = getpagesize();
> > > +       unsigned long size = 2 * page_size;
> > > +       int ret;
> > > +       int prot;
> > > +
> > > +       /*
> > > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > > +        * It might mprotect the first, but it'll never touch the second (msealed) vma.
> > > +        */
> > > +
> > > +       setup_single_address(size, &ptr);
> > > +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > +
> > > +       if (seal) {
> > > +               ret = sys_mseal(ptr + page_size, size);
> > you are allocating 2 pages , and I assume you are sealing the second
> > page, so the size should be page_size.
> > ret = sys_mseal(ptr + page_size, page_size);
>
> Yes, good catch, it appears to be harmless but ofc down to straight luck.
> I'll send a fixup for this and the other mistake down there.
>
> >
> > > +               FAIL_TEST_IF_FALSE(!ret);
> > > +       }
> > > +
> > > +       ret = sys_mprotect(ptr, size, PROT_EXEC);
> > > +       if (seal)
> > > +               FAIL_TEST_IF_FALSE(ret < 0);
> > > +       else
> > > +               FAIL_TEST_IF_FALSE(!ret);
> > > +
> > > +       if (seal) {
> > > +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> > > +               FAIL_TEST_IF_FALSE(prot == 0x4);
> > To test partial mprotect, the test needs to add the check for the
> > first page to be changed, Also to avoid the merge,  a PROT_NONE page
> > can to be added in front.
>
> No, I'm leaving partial mprotect to be undefined. It doesn't make
> sense to constraint ourselves, since POSIX wording is already loose.
>
> >
> > > +       }
> > > +
> > > +       REPORT_TEST_PASS();
> > > +}
> > > +
> > > +
> > >  static void test_seal_mprotect_two_vma_with_gap(bool seal)
> > >  {
> > >         void *ptr;
> > > @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
> > >         REPORT_TEST_PASS();
> > >  }
> > >
> > > +static void test_seal_munmap_partial_across_vmas(bool seal)
> > > +{
> > > +       void *ptr;
> > > +       unsigned long page_size = getpagesize();
> > > +       unsigned long size = 2 * page_size;
> > > +       int ret;
> > > +       int prot;
> > > +
> > > +       /*
> > > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > > +        * It might unmap the first, but it'll never unmap the second (msealed) vma.
> > > +        */
> > > +
> > > +       setup_single_address(size, &ptr);
> > > +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > +
> > > +       if (seal) {
> > > +               ret = sys_mseal(ptr + page_size, size);
> > ret = sys_mseal(ptr + page_size, page_size);
> >
> > > +               FAIL_TEST_IF_FALSE(!ret);
> > > +       }
> > > +
> > > +       ret = sys_munmap(ptr, size);
> > > +       if (seal)
> > > +               FAIL_TEST_IF_FALSE(ret < 0);
> > > +       else
> > > +               FAIL_TEST_IF_FALSE(!ret);
> > > +
> > > +       if (seal) {
> > > +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> > > +               FAIL_TEST_IF_FALSE(prot == 0x4);
> > To test partial unmap, the test needs to add the check for the first
> > page to be freed, Also to avoid the merge,  a PROT_NONE page needs to
> > be in front.
>
> I'm not testing partial unmap. Partial unmap does not happen. I have
> told you this before.
>
ok.  Then this test should be as below ? (need to add PROT_NONE page
before and after)
  size = get_vma_size(ptr, &prot);
  FAIL_TEST_IF_FALSE(size == 2 * page_size);
  FAIL_TEST_IF_FALSE(prot==0x4)


> >
> > The test_seal_munmap_partial_across_vmas  shows the behavior
> > difference with in-loop approach and out-loop approach. Previously,
> > both VMAs will not be freed, now the first VMA will be freed, and the
> > second VMA (sealed) won't.
> >
> > This brings to the line you previously mentioned: [1] and I quote:
> > "munmap is atomic and always has been. It's required by POSIX."
>
> This is still true, the comment was a copy-and-paste mindslip. Please
> read the email thread. It has been fixed up by Andrew.
>
Which thread/patch by Andrew ? Could you please send it to me ? (I
might missed that)

> --
> Pedro
Pedro Falcato Aug. 21, 2024, 5:28 p.m. UTC | #7
On Wed, Aug 21, 2024 at 5:27 PM Jeff Xu <jeffxu@chromium.org> wrote:
>
> On Wed, Aug 21, 2024 at 9:20 AM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Wed, Aug 21, 2024 at 4:56 PM Jeff Xu <jeffxu@chromium.org> wrote:
> > >
> > > Hi Pedro
> > >
> > > On Fri, Aug 16, 2024 at 5:18 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > > >
> > > > Add more mseal traversal tests across VMAs, where we could possibly
> > > > screw up sealing checks. These test more across-vma traversal for
> > > > mprotect, munmap and madvise. Particularly, we test for the case where a
> > > > regular VMA is followed by a sealed VMA.
> > > >
> > > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > > > ---
> > > >  tools/testing/selftests/mm/mseal_test.c | 111 +++++++++++++++++++++++++++++++-
> > > >  1 file changed, 110 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > > > index 259bef4945e9..0d4d40fb0f88 100644
> > > > --- a/tools/testing/selftests/mm/mseal_test.c
> > > > +++ b/tools/testing/selftests/mm/mseal_test.c
> > > > @@ -766,6 +766,42 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
> > > >         REPORT_TEST_PASS();
> > > >  }
> > > >
> > > > +static void test_seal_mprotect_partial_mprotect_tail(bool seal)
> > > > +{
> > > > +       void *ptr;
> > > > +       unsigned long page_size = getpagesize();
> > > > +       unsigned long size = 2 * page_size;
> > > > +       int ret;
> > > > +       int prot;
> > > > +
> > > > +       /*
> > > > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > > > +        * It might mprotect the first, but it'll never touch the second (msealed) vma.
> > > > +        */
> > > > +
> > > > +       setup_single_address(size, &ptr);
> > > > +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > +
> > > > +       if (seal) {
> > > > +               ret = sys_mseal(ptr + page_size, size);
> > > you are allocating 2 pages , and I assume you are sealing the second
> > > page, so the size should be page_size.
> > > ret = sys_mseal(ptr + page_size, page_size);
> >
> > Yes, good catch, it appears to be harmless but ofc down to straight luck.
> > I'll send a fixup for this and the other mistake down there.
> >
> > >
> > > > +               FAIL_TEST_IF_FALSE(!ret);
> > > > +       }
> > > > +
> > > > +       ret = sys_mprotect(ptr, size, PROT_EXEC);
> > > > +       if (seal)
> > > > +               FAIL_TEST_IF_FALSE(ret < 0);
> > > > +       else
> > > > +               FAIL_TEST_IF_FALSE(!ret);
> > > > +
> > > > +       if (seal) {
> > > > +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> > > > +               FAIL_TEST_IF_FALSE(prot == 0x4);
> > > To test partial mprotect, the test needs to add the check for the
> > > first page to be changed, Also to avoid the merge,  a PROT_NONE page
> > > can to be added in front.
> >
> > No, I'm leaving partial mprotect to be undefined. It doesn't make
> > sense to constraint ourselves, since POSIX wording is already loose.
> >
> > >
> > > > +       }
> > > > +
> > > > +       REPORT_TEST_PASS();
> > > > +}
> > > > +
> > > > +
> > > >  static void test_seal_mprotect_two_vma_with_gap(bool seal)
> > > >  {
> > > >         void *ptr;
> > > > @@ -983,6 +1019,41 @@ static void test_seal_munmap_vma_with_gap(bool seal)
> > > >         REPORT_TEST_PASS();
> > > >  }
> > > >
> > > > +static void test_seal_munmap_partial_across_vmas(bool seal)
> > > > +{
> > > > +       void *ptr;
> > > > +       unsigned long page_size = getpagesize();
> > > > +       unsigned long size = 2 * page_size;
> > > > +       int ret;
> > > > +       int prot;
> > > > +
> > > > +       /*
> > > > +        * Check if a partial mseal (that results in two vmas) works correctly.
> > > > +        * It might unmap the first, but it'll never unmap the second (msealed) vma.
> > > > +        */
> > > > +
> > > > +       setup_single_address(size, &ptr);
> > > > +       FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > > > +
> > > > +       if (seal) {
> > > > +               ret = sys_mseal(ptr + page_size, size);
> > > ret = sys_mseal(ptr + page_size, page_size);
> > >
> > > > +               FAIL_TEST_IF_FALSE(!ret);
> > > > +       }
> > > > +
> > > > +       ret = sys_munmap(ptr, size);
> > > > +       if (seal)
> > > > +               FAIL_TEST_IF_FALSE(ret < 0);
> > > > +       else
> > > > +               FAIL_TEST_IF_FALSE(!ret);
> > > > +
> > > > +       if (seal) {
> > > > +               FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
> > > > +               FAIL_TEST_IF_FALSE(prot == 0x4);
> > > To test partial unmap, the test needs to add the check for the first
> > > page to be freed, Also to avoid the merge,  a PROT_NONE page needs to
> > > be in front.
> >
> > I'm not testing partial unmap. Partial unmap does not happen. I have
> > told you this before.
> >
> ok.  Then this test should be as below ? (need to add PROT_NONE page
> before and after)
>   size = get_vma_size(ptr, &prot);
>   FAIL_TEST_IF_FALSE(size == 2 * page_size);
>   FAIL_TEST_IF_FALSE(prot==0x4)

That doesn't work because this region spans two vmas. I'll write
something similar for the fixup.

>
>
> > >
> > > The test_seal_munmap_partial_across_vmas  shows the behavior
> > > difference with in-loop approach and out-loop approach. Previously,
> > > both VMAs will not be freed, now the first VMA will be freed, and the
> > > second VMA (sealed) won't.
> > >
> > > This brings to the line you previously mentioned: [1] and I quote:
> > > "munmap is atomic and always has been. It's required by POSIX."
> >
> > This is still true, the comment was a copy-and-paste mindslip. Please
> > read the email thread. It has been fixed up by Andrew.
> >
> Which thread/patch by Andrew ? Could you please send it to me ? (I
> might missed that)

This thread and this patch:
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/selftests-mm-add-more-mseal-traversal-tests-fix.patch
Pedro Falcato Aug. 21, 2024, 5:36 p.m. UTC | #8
On Wed, Aug 21, 2024 at 6:28 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > ok.  Then this test should be as below ? (need to add PROT_NONE page
> > before and after)
> >   size = get_vma_size(ptr, &prot);
> >   FAIL_TEST_IF_FALSE(size == 2 * page_size);
> >   FAIL_TEST_IF_FALSE(prot==0x4)
>
> That doesn't work because this region spans two vmas. I'll write
> something similar for the fixup.

Actually, I won't because this might cause spurious test failures on
e.g !TOPDOWN mmap architectures.
setup_single_address (and co) need a fresh coat of paint (wrt
PROT_NONE guard regions around it) and I don't want to be the one to
do it, at least not as part of this series.
Pedro Falcato Aug. 21, 2024, 11:37 p.m. UTC | #9
Andrew, please squash this small patch with this one. It directly addresses
a problem found in review.

(I was told this is the preferred way to send small fixups, and I don't
think anyone wants a v4 over this trivial issue)

Thank you!

----8<----
From 614e5dc27073c39579c863ebdff4396948e28e03 Mon Sep 17 00:00:00 2001
From: Pedro Falcato <pedro.falcato@gmail.com>
Date: Thu, 22 Aug 2024 00:20:19 +0100
Subject: [PATCH] selftests/mm: Fix mseal's length

We accidentally msealed too much, which could overrun and try to mseal
other regions. This seems to be harmless (at least on top-down
architectures) due to various reasons all aligning, but may very well
cause spurious test failures to e.g bottom-up mmap architectures.

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 tools/testing/selftests/mm/mseal_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index 0d4d40fb0f88..0c41513219ae 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -783,7 +783,7 @@ static void test_seal_mprotect_partial_mprotect_tail(bool seal)
        FAIL_TEST_IF_FALSE(ptr != (void *)-1);
 
        if (seal) {
-               ret = sys_mseal(ptr + page_size, size);
+               ret = sys_mseal(ptr + page_size, page_size);
                FAIL_TEST_IF_FALSE(!ret);
        }
 
@@ -1036,7 +1036,7 @@ static void test_seal_munmap_partial_across_vmas(bool seal)
        FAIL_TEST_IF_FALSE(ptr != (void *)-1);
 
        if (seal) {
-               ret = sys_mseal(ptr + page_size, size);
+               ret = sys_mseal(ptr + page_size, page_size);
                FAIL_TEST_IF_FALSE(!ret);
        }
diff mbox series

Patch

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index 259bef4945e9..0d4d40fb0f88 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -766,6 +766,42 @@  static void test_seal_mprotect_partial_mprotect(bool seal)
 	REPORT_TEST_PASS();
 }
 
+static void test_seal_mprotect_partial_mprotect_tail(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 2 * page_size;
+	int ret;
+	int prot;
+
+	/*
+	 * Check if a partial mseal (that results in two vmas) works correctly.
+	 * It might mprotect the first, but it'll never touch the second (msealed) vma.
+	 */
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	if (seal) {
+		ret = sys_mseal(ptr + page_size, size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	ret = sys_mprotect(ptr, size, PROT_EXEC);
+	if (seal)
+		FAIL_TEST_IF_FALSE(ret < 0);
+	else
+		FAIL_TEST_IF_FALSE(!ret);
+
+	if (seal) {
+		FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	}
+
+	REPORT_TEST_PASS();
+}
+
+
 static void test_seal_mprotect_two_vma_with_gap(bool seal)
 {
 	void *ptr;
@@ -983,6 +1019,41 @@  static void test_seal_munmap_vma_with_gap(bool seal)
 	REPORT_TEST_PASS();
 }
 
+static void test_seal_munmap_partial_across_vmas(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 2 * page_size;
+	int ret;
+	int prot;
+
+	/*
+	 * Check if a partial mseal (that results in two vmas) works correctly.
+	 * It might unmap the first, but it'll never unmap the second (msealed) vma.
+	 */
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	if (seal) {
+		ret = sys_mseal(ptr + page_size, size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	ret = sys_munmap(ptr, size);
+	if (seal)
+		FAIL_TEST_IF_FALSE(ret < 0);
+	else
+		FAIL_TEST_IF_FALSE(!ret);
+
+	if (seal) {
+		FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	}
+
+	REPORT_TEST_PASS();
+}
+
 static void test_munmap_start_freed(bool seal)
 {
 	void *ptr;
@@ -1735,6 +1806,37 @@  static void test_seal_discard_ro_anon(bool seal)
 	REPORT_TEST_PASS();
 }
 
+static void test_seal_discard_across_vmas(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 2 * page_size;
+	int ret;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	if (seal) {
+		ret = seal_single_address(ptr + page_size, page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	ret = sys_madvise(ptr, size, MADV_DONTNEED);
+	if (seal)
+		FAIL_TEST_IF_FALSE(ret < 0);
+	else
+		FAIL_TEST_IF_FALSE(!ret);
+
+	ret = sys_munmap(ptr, size);
+	if (seal)
+		FAIL_TEST_IF_FALSE(ret < 0);
+	else
+		FAIL_TEST_IF_FALSE(!ret);
+
+	REPORT_TEST_PASS();
+}
+
+
 static void test_seal_madvise_nodiscard(bool seal)
 {
 	void *ptr;
@@ -1779,7 +1881,7 @@  int main(int argc, char **argv)
 	if (!pkey_supported())
 		ksft_print_msg("PKEY not supported\n");
 
-	ksft_set_plan(82);
+	ksft_set_plan(88);
 
 	test_seal_addseal();
 	test_seal_unmapped_start();
@@ -1825,12 +1927,17 @@  int main(int argc, char **argv)
 	test_seal_mprotect_split(false);
 	test_seal_mprotect_split(true);
 
+	test_seal_mprotect_partial_mprotect_tail(false);
+	test_seal_mprotect_partial_mprotect_tail(true);
+
 	test_seal_munmap(false);
 	test_seal_munmap(true);
 	test_seal_munmap_two_vma(false);
 	test_seal_munmap_two_vma(true);
 	test_seal_munmap_vma_with_gap(false);
 	test_seal_munmap_vma_with_gap(true);
+	test_seal_munmap_partial_across_vmas(false);
+	test_seal_munmap_partial_across_vmas(true);
 
 	test_munmap_start_freed(false);
 	test_munmap_start_freed(true);
@@ -1862,6 +1969,8 @@  int main(int argc, char **argv)
 	test_seal_madvise_nodiscard(true);
 	test_seal_discard_ro_anon(false);
 	test_seal_discard_ro_anon(true);
+	test_seal_discard_across_vmas(false);
+	test_seal_discard_across_vmas(true);
 	test_seal_discard_ro_anon_on_rw(false);
 	test_seal_discard_ro_anon_on_rw(true);
 	test_seal_discard_ro_anon_on_shared(false);