Message-ID: <e7ea2b84-8d10-40fe-a14f-837bca851ea9@lucifer.local>
Date: Fri, 30 Aug 2024 19:43:12 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: jeffxu@...omium.org
Cc: akpm@...ux-foundation.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org, pedro.falcato@...il.com,
        willy@...radead.org, broonie@...nel.org, vbabka@...e.cz,
        Liam.Howlett@...cle.com, rientjes@...gle.com, keescook@...omium.org
Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@...omium.org wrote:
> From: Jeff Xu <jeffxu@...omium.org>
>
> Add sealing test to cover mmap for
> Expand/shrink across sealed vmas (MAP_FIXED)
> Reuse the same address in !MAP_FIXED case.

This commit message is woefully small. I told you on v1 to improve the
commit messages. Linus has told you to do this before.

Please actually respond to feedback. Thanks.

>
> Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> ---
>  tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index e855c8ccefc3..3516389034a7 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal)
>  	REPORT_TEST_PASS();
>  }
>
> +static void test_seal_mmap_expand_seal_middle(bool seal)

This test doesn't expand, doesn't do anything in the middle. It does mmap()
though and relates to mseal, so that's something... this is compeltely
misnamed and needs to be rethought.

> +{
> +	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.

> +	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().

> +
> +	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.

> +	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.

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.

> +
> +	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?
> +{
> +	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.

> +	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??

> +
> +		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?

So what are we testing here exactly? That we got a VMA split? This is
err... why are we asserting this?

> +	} 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.

> +{
> +	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!

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.

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?

>
>  	test_seal_addseal();
>  	test_seal_unmapped_start();
> @@ -2357,5 +2474,12 @@ int main(int argc, char **argv)
>  	test_munmap_free_multiple_ranges(false);
>  	test_munmap_free_multiple_ranges(true);
>
> +	test_seal_mmap_expand_seal_middle(false);
> +	test_seal_mmap_expand_seal_middle(true);
> +	test_seal_mmap_shrink_seal_middle(false);
> +	test_seal_mmap_shrink_seal_middle(true);
> +	test_seal_mmap_reuse_addr(false);
> +	test_seal_mmap_reuse_addr(true);
> +
>  	ksft_finished();
>  }
> --
> 2.46.0.469.g59c65b2a67-goog
>

Powered by blists - more mailing lists