[PATCH v3 0/5] Increase mseal test coverage

[PATCH v3 0/5] Increase mseal test coverage

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

This series increase the test coverage of mseal_test by:

Add check for vma_size, prot, and error code for existing tests.
Add more testcases for madvise, munmap, mmap and mremap to cover
sealing in different scenarios.

The increase test coverage hopefully help to prevent future regression.
It doesn't change any existing mm api's semantics, i.e. it will pass on
linux main and 6.10 branch.

Note: in order to pass this test in mm-unstable, mm-unstable must have
Liam's fix on mmap [1]

[1] https://lore.kernel.org/linux-kselftest/vyllxuh5xbqmaoyl2mselebij5ox7cseekjcvl5gmzoxxwd2he@hxi4mpjanxzt/#t

History:
V3:
- no-functional change, incooperate feedback from Pedro Falcato

V2:
- https://lore.kernel.org/linux-kselftest/20240829214352.963001-1-jeffxu@chromium.org/
- remove the mmap fix (Liam R. Howlett will fix it separately)
- Add cover letter (Lorenzo Stoakes)
- split the testcase for ease of review (Mark Brown)

V1:
- https://lore.kernel.org/linux-kselftest/20240828225522.684774-1-jeffxu@chromium.org/


Jeff Xu (5):
  selftests/mseal_test: Check vma_size, prot, error code.
  selftests/mseal: add sealed madvise type
  selftests/mseal: munmap across multiple vma ranges.
  selftests/mseal: add more tests for mmap
  selftests/mseal: add more tests for mremap

 tools/testing/selftests/mm/mseal_test.c | 830 ++++++++++++++++++++++--
 1 file changed, 763 insertions(+), 67 deletions(-)

-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v3 1/5] selftests/mseal_test: Check vma_size, prot, error code.

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Add more checks for vma size, prot bits and api errcode
after attempt of modifing sealed mappings.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 tools/testing/selftests/mm/mseal_test.c | 398 ++++++++++++++++++++----
 1 file changed, 332 insertions(+), 66 deletions(-)

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index e7991e5fdcf3..7198f2314f9b 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -170,18 +170,31 @@ static void set_pkey(int pkey, unsigned long pkey_value)
 static void setup_single_address(int size, void **ptrOut)
 {
 	void *ptr;
+	unsigned long page_size = getpagesize();
 
-	ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
-	*ptrOut = ptr;
+	*ptrOut = (void *) MAP_FAILED;
+	ptr = mmap(NULL, size + 2 * page_size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+	if (ptr != (void *) -1) {
+		/* add 2 page at the beginning and end to avoid auto-merge of mapping */
+		sys_mprotect(ptr, page_size, PROT_NONE);
+		sys_mprotect(ptr + size + page_size, page_size, PROT_NONE);
+		*ptrOut = ptr + page_size;
+	}
 }
 
 static void setup_single_address_rw(int size, void **ptrOut)
 {
 	void *ptr;
 	unsigned long mapflags = MAP_ANONYMOUS | MAP_PRIVATE;
+	unsigned long page_size = getpagesize();
 
-	ptr = mmap(NULL, size, PROT_READ | PROT_WRITE, mapflags, -1, 0);
-	*ptrOut = ptr;
+	*ptrOut = (void *) MAP_FAILED;
+	ptr = mmap(NULL, size + 2 * page_size, PROT_READ | PROT_WRITE, mapflags, -1, 0);
+	if (ptr != (void *) -1) {
+		sys_mprotect(ptr, page_size, PROT_NONE);
+		sys_mprotect(ptr + size + page_size, page_size, PROT_NONE);
+		*ptrOut = ptr + page_size;
+	}
 }
 
 static int clean_single_address(void *ptr, int size)
@@ -226,6 +239,21 @@ bool pkey_supported(void)
 	return false;
 }
 
+bool get_vma_size_supported(void)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 4 * page_size;
+	int prot;
+
+	setup_single_address(size, &ptr);
+	size = get_vma_size(ptr, &prot);
+	if (size == 4 * page_size && prot == 0x4)
+		return true;
+
+	return false;
+}
+
 static void test_seal_addseal(void)
 {
 	int ret;
@@ -419,11 +447,17 @@ static void test_seal_invalid_input(void)
 	unsigned long size = 4 * page_size;
 	int ret;
 
-	setup_single_address(8 * page_size, &ptr);
+	setup_single_address(9 * page_size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
-	ret = clean_single_address(ptr + 4 * page_size, 4 * page_size);
+
+	ret = clean_single_address(ptr, page_size);
 	FAIL_TEST_IF_FALSE(!ret);
 
+	ret = clean_single_address(ptr + 5 * page_size, 4 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	ptr = ptr + page_size;
+
 	/* invalid flag */
 	ret = syscall(__NR_mseal, ptr, size, 0x20);
 	FAIL_TEST_IF_FALSE(ret < 0);
@@ -523,6 +557,7 @@ static void test_seal_mprotect(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -533,9 +568,14 @@ static void test_seal_mprotect(bool seal)
 	}
 
 	ret = sys_mprotect(ptr, size, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		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);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -547,6 +587,7 @@ static void test_seal_start_mprotect(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -558,9 +599,14 @@ static void test_seal_start_mprotect(bool seal)
 
 	/* the first page is sealed. */
 	ret = sys_mprotect(ptr, page_size, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	/* pages after the first page is not sealed. */
@@ -577,6 +623,7 @@ static void test_seal_end_mprotect(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -593,9 +640,14 @@ static void test_seal_end_mprotect(bool seal)
 	/* last 3 page are sealed */
 	ret = sys_mprotect(ptr + page_size, page_size * 3,
 			PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr + page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -607,6 +659,7 @@ static void test_seal_mprotect_unalign_len(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -618,9 +671,14 @@ static void test_seal_mprotect_unalign_len(bool seal)
 
 	/* 2 pages are sealed. */
 	ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	ret = sys_mprotect(ptr + page_size * 2, page_size,
@@ -636,6 +694,7 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -646,9 +705,14 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal)
 
 	/* 3 pages are sealed. */
 	ret = sys_mprotect(ptr, page_size * 3, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	ret = sys_mprotect(ptr + page_size * 3, page_size,
@@ -664,6 +728,7 @@ static void test_seal_mprotect_two_vma(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -678,16 +743,26 @@ static void test_seal_mprotect_two_vma(bool seal)
 	}
 
 	ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	ret = sys_mprotect(ptr + page_size * 2, page_size * 2,
 			PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr + page_size * 2, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -699,6 +774,7 @@ static void test_seal_mprotect_two_vma_with_split(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -719,17 +795,27 @@ static void test_seal_mprotect_two_vma_with_split(bool seal)
 
 	/* the second page is sealed. */
 	ret = sys_mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	/* the third page is sealed. */
 	ret = sys_mprotect(ptr + 2 * page_size, page_size,
 			PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr + 2 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	/* the fouth page is not sealed. */
@@ -746,6 +832,7 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -758,9 +845,14 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
 
 	/* mprotect first 2 page will fail, since the first page are sealed. */
 	ret = sys_mprotect(ptr, 2 * page_size, PROT_READ | PROT_WRITE);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -783,15 +875,15 @@ static void test_seal_mprotect_partial_mprotect_tail(bool seal)
 	}
 
 	ret = sys_mprotect(ptr, size, PROT_EXEC);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
-		FAIL_TEST_IF_FALSE(!ret);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
 
-	if (seal) {
-		FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
+		size = get_vma_size(ptr + page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
 		FAIL_TEST_IF_FALSE(prot == 0x4);
-	}
+	} else
+		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
 }
@@ -846,6 +938,7 @@ static void test_seal_mprotect_split(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -862,16 +955,34 @@ static void test_seal_mprotect_split(bool seal)
 
 	/* mprotect is sealed. */
 	ret = sys_mprotect(ptr, 2 * page_size, PROT_READ);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+
+		size = get_vma_size(ptr + page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 
 	ret = sys_mprotect(ptr + 2 * page_size, 2 * page_size, PROT_READ);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+
+		size = get_vma_size(ptr + page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -883,6 +994,7 @@ static void test_seal_mprotect_merge(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -899,9 +1011,18 @@ static void test_seal_mprotect_merge(bool seal)
 
 	/* 2 pages are sealed. */
 	ret = sys_mprotect(ptr, 2 * page_size, PROT_READ);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+
+		size = get_vma_size(ptr + page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	/* last 2 pages are not sealed. */
@@ -917,6 +1038,7 @@ static void test_seal_munmap(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -928,9 +1050,14 @@ static void test_seal_munmap(bool seal)
 
 	/* 4 pages are sealed. */
 	ret = sys_munmap(ptr, size);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		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);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -948,6 +1075,7 @@ static void test_seal_munmap_two_vma(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -962,15 +1090,33 @@ static void test_seal_munmap_two_vma(bool seal)
 	}
 
 	ret = sys_munmap(ptr, page_size * 2);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+
+		size = get_vma_size(ptr + 2 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	ret = sys_munmap(ptr + page_size, page_size * 2);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x6);
+
+		size = get_vma_size(ptr + 2 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -1018,33 +1164,75 @@ static void test_seal_munmap_partial_across_vmas(bool seal)
 {
 	void *ptr;
 	unsigned long page_size = getpagesize();
-	unsigned long size = 2 * page_size;
+	unsigned long size = 12 * 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, page_size);
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
 		FAIL_TEST_IF_FALSE(!ret);
 	}
 
-	ret = sys_munmap(ptr, size);
-	if (seal)
+	ret = sys_munmap(ptr, 12 * page_size);
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		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);
+
+		size = get_vma_size(ptr + 8 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
+
+	ret = sys_munmap(ptr, 6 * page_size);
 	if (seal) {
-		FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0);
+		FAIL_TEST_IF_FALSE(ret < 0);
+		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);
-	}
+
+		size = get_vma_size(ptr + 8 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
+		FAIL_TEST_IF_FALSE(!ret);
+
+	ret = sys_munmap(ptr + 6 * page_size, 6 * page_size);
+	if (seal) {
+		FAIL_TEST_IF_FALSE(ret < 0);
+		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);
+
+		size = get_vma_size(ptr + 8 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
+		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
 }
@@ -1074,9 +1262,11 @@ static void test_munmap_start_freed(bool seal)
 	ret = sys_munmap(ptr, size);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
 
 		size = get_vma_size(ptr + page_size, &prot);
-		FAIL_TEST_IF_FALSE(size == page_size * 3);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else {
 		/* note: this will be OK, even the first page is */
 		/* already unmapped. */
@@ -1095,6 +1285,7 @@ static void test_munmap_end_freed(bool seal)
 	unsigned long page_size = getpagesize();
 	unsigned long size = 4 * page_size;
 	int ret;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1111,9 +1302,14 @@ static void test_munmap_end_freed(bool seal)
 
 	/* unmap all pages. */
 	ret = sys_munmap(ptr, size);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	REPORT_TEST_PASS();
@@ -1144,12 +1340,15 @@ static void test_munmap_middle_freed(bool seal)
 	ret = sys_munmap(ptr, size);
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
 
 		size = get_vma_size(ptr, &prot);
 		FAIL_TEST_IF_FALSE(size == page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 
 		size = get_vma_size(ptr + page_size * 3, &prot);
 		FAIL_TEST_IF_FALSE(size == page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else {
 		FAIL_TEST_IF_FALSE(!ret);
 
@@ -1170,6 +1369,7 @@ static void test_seal_mremap_shrink(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1184,6 +1384,10 @@ static void test_seal_mremap_shrink(bool seal)
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == (void *) 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);
 	} else {
 		FAIL_TEST_IF_FALSE(ret2 != (void *) MAP_FAILED);
 
@@ -1199,6 +1403,7 @@ static void test_seal_mremap_expand(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1216,6 +1421,10 @@ static void test_seal_mremap_expand(bool seal)
 	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 == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else {
 		FAIL_TEST_IF_FALSE(ret2 == ptr);
 
@@ -1231,6 +1440,7 @@ static void test_seal_mremap_move(bool seal)
 	unsigned long size = page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1249,10 +1459,12 @@ static void test_seal_mremap_move(bool seal)
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
-	} else {
-		FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
 
-	}
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size ==  page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
+		FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
 
 	REPORT_TEST_PASS();
 }
@@ -1264,6 +1476,7 @@ static void test_seal_mmap_overwrite_prot(bool seal)
 	unsigned long size = page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1279,6 +1492,10 @@ static void test_seal_mmap_overwrite_prot(bool seal)
 	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 == page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else
 		FAIL_TEST_IF_FALSE(ret2 == ptr);
 
@@ -1292,6 +1509,7 @@ static void test_seal_mmap_expand(bool seal)
 	unsigned long size = 12 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1310,6 +1528,10 @@ static void test_seal_mmap_expand(bool seal)
 	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 == 8 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else
 		FAIL_TEST_IF_FALSE(ret2 == ptr);
 
@@ -1323,6 +1545,7 @@ static void test_seal_mmap_shrink(bool seal)
 	unsigned long size = 12 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1338,6 +1561,10 @@ static void test_seal_mmap_shrink(bool seal)
 	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 == 12 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else
 		FAIL_TEST_IF_FALSE(ret2 == ptr);
 
@@ -1352,6 +1579,7 @@ static void test_seal_mremap_shrink_fixed(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1369,6 +1597,10 @@ static void test_seal_mremap_shrink_fixed(bool seal)
 	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);
 	} else
 		FAIL_TEST_IF_FALSE(ret2 == newAddr);
 
@@ -1383,6 +1615,7 @@ static void test_seal_mremap_expand_fixed(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(page_size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1400,6 +1633,10 @@ static void test_seal_mremap_expand_fixed(bool seal)
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(newAddr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else
 		FAIL_TEST_IF_FALSE(ret2 == newAddr);
 
@@ -1414,6 +1651,7 @@ static void test_seal_mremap_move_fixed(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1430,6 +1668,10 @@ static void test_seal_mremap_move_fixed(bool seal)
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(newAddr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
 	} else
 		FAIL_TEST_IF_FALSE(ret2 == newAddr);
 
@@ -1443,6 +1685,7 @@ static void test_seal_mremap_move_fixed_zero(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1460,9 +1703,12 @@ static void test_seal_mremap_move_fixed_zero(bool seal)
 	if (seal) {
 		FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);
 		FAIL_TEST_IF_FALSE(errno == EPERM);
-	} else {
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 0x4);
+	} else
 		FAIL_TEST_IF_FALSE(ret2 == 0);
-	}
 
 	REPORT_TEST_PASS();
 }
@@ -1474,6 +1720,7 @@ static void test_seal_mremap_move_dontunmap(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1488,6 +1735,10 @@ static void test_seal_mremap_move_dontunmap(bool seal)
 	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);
 	} else {
 		/* kernel will allocate a new address */
 		FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
@@ -1503,6 +1754,7 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal)
 	unsigned long size = 4 * page_size;
 	int ret;
 	void *ret2;
+	int prot;
 
 	setup_single_address(size, &ptr);
 	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
@@ -1529,6 +1781,10 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal)
 	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);
 	} else {
 		/* remap success and return ptr2 */
 		FAIL_TEST_IF_FALSE(ret2 ==  ptr2);
@@ -1690,9 +1946,10 @@ static void test_seal_discard_ro_anon_on_pkey(bool seal)
 	/* sealing will take effect if PKRU deny write. */
 	set_pkey(pkey, PKEY_DISABLE_WRITE);
 	ret = sys_madvise(ptr, size, MADV_DONTNEED);
-	if (seal)
+	if (seal) {
 		FAIL_TEST_IF_FALSE(ret < 0);
-	else
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+	} else
 		FAIL_TEST_IF_FALSE(!ret);
 
 	/* base seal still apply. */
@@ -1876,6 +2133,15 @@ int main(int argc, char **argv)
 	if (!pkey_supported())
 		ksft_print_msg("PKEY not supported\n");
 
+	/*
+	 * Possible reasons:
+	 * - unable to read /proc/pid/maps (unlikely)
+	 * - parsing error when reading /proc/pid/maps,e.g. len is not expected.
+	 *   Is this "TOPDOWN" mapping or format change in /proc/pid/maps ?
+	 */
+	if (!get_vma_size_supported())
+		ksft_exit_skip("get_vma_size not supported\n");
+
 	ksft_set_plan(88);
 
 	test_seal_addseal();
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v3 2/5] selftests/mseal: add sealed madvise type

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Add a testcase to cover all sealed madvise type.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 tools/testing/selftests/mm/mseal_test.c | 30 ++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index 7198f2314f9b..6d77dc9b5442 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -2121,6 +2121,32 @@ static void test_seal_madvise_nodiscard(bool seal)
 	REPORT_TEST_PASS();
 }
 
+static void test_seal_discard_madvise_advice(void)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 4 * page_size;
+	int ret;
+	int sealed_advice[] = {MADV_FREE, MADV_DONTNEED,
+		MADV_DONTNEED_LOCKED, MADV_REMOVE,
+		MADV_DONTFORK, MADV_WIPEONFORK};
+	int size_sealed_advice = sizeof(sealed_advice) / sizeof(int);
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	ret = seal_single_address(ptr, size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	for (int i = 0; i < size_sealed_advice; i++) {
+		ret = sys_madvise(ptr, size, sealed_advice[i]);
+		FAIL_TEST_IF_FALSE(ret < 0);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+	}
+
+	REPORT_TEST_PASS();
+}
+
 int main(int argc, char **argv)
 {
 	bool test_seal = seal_support();
@@ -2142,7 +2168,7 @@ int main(int argc, char **argv)
 	if (!get_vma_size_supported())
 		ksft_exit_skip("get_vma_size not supported\n");
 
-	ksft_set_plan(88);
+	ksft_set_plan(89);
 
 	test_seal_addseal();
 	test_seal_unmapped_start();
@@ -2251,5 +2277,7 @@ int main(int argc, char **argv)
 	test_seal_discard_ro_anon_on_pkey(false);
 	test_seal_discard_ro_anon_on_pkey(true);
 
+	test_seal_discard_madvise_advice();
+
 	ksft_finished();
 }
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v3 3/5] selftests/mseal: munmap across multiple vma ranges.

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Add a test to munmap across multiple vma ranges.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 tools/testing/selftests/mm/mseal_test.c | 80 ++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index 6d77dc9b5442..e855c8ccefc3 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -2147,6 +2147,81 @@ static void test_seal_discard_madvise_advice(void)
 	REPORT_TEST_PASS();
 }
 
+static void test_munmap_free_multiple_ranges(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 8 * page_size;
+	int ret;
+	int prot;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	/* unmap one page from beginning. */
+	ret = sys_munmap(ptr, page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	/* unmap one page from middle. */
+	ret = sys_munmap(ptr + 4 * page_size, page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	size = get_vma_size(ptr + page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 3 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 4);
+
+	size = get_vma_size(ptr +  5 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 3 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 4);
+
+
+	/* seal the last page */
+	if (seal) {
+		ret = sys_mseal(ptr + 7 * page_size, page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+
+		size = get_vma_size(ptr +  1 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 4);
+
+		size = get_vma_size(ptr +  5 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 4);
+
+		size = get_vma_size(ptr +  7 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 4);
+	}
+
+	/* munmap all 8  pages from beginning */
+	ret = sys_munmap(ptr, 8 * page_size);
+	if (seal) {
+		FAIL_TEST_IF_FALSE(ret);
+
+		/* verify mapping are not changed */
+		size = get_vma_size(ptr + 1 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 3 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 4);
+
+		size = get_vma_size(ptr +  5 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 4);
+
+		size = get_vma_size(ptr +  7 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 1 * page_size);
+		FAIL_TEST_IF_FALSE(prot == 4);
+	} else {
+		FAIL_TEST_IF_FALSE(!ret);
+
+		for (int i = 0; i < 8; i++) {
+			size = get_vma_size(ptr, &prot);
+			FAIL_TEST_IF_FALSE(size == 0);
+		}
+	}
+
+	REPORT_TEST_PASS();
+}
+
 int main(int argc, char **argv)
 {
 	bool test_seal = seal_support();
@@ -2168,7 +2243,7 @@ int main(int argc, char **argv)
 	if (!get_vma_size_supported())
 		ksft_exit_skip("get_vma_size not supported\n");
 
-	ksft_set_plan(89);
+	ksft_set_plan(91);
 
 	test_seal_addseal();
 	test_seal_unmapped_start();
@@ -2279,5 +2354,8 @@ int main(int argc, char **argv)
 
 	test_seal_discard_madvise_advice();
 
+	test_munmap_free_multiple_ranges(false);
+	test_munmap_free_multiple_ranges(true);
+
 	ksft_finished();
 }
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH v3 4/5] selftests/mseal: add more tests for mmap

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Add sealing test to cover mmap for
Expand/shrink across sealed vmas (MAP_FIXED)
Reuse the same address in !MAP_FIXED case.

Signed-off-by: Jeff Xu <jeffxu@chromium.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)
+{
+	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);
+	/* ummap last 4 pages. */
+	ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	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)  */
+	ret2 = mmap(ptr, 12 * 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);
+
+		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);
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mmap_shrink_seal_middle(bool seal)
+{
+	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)  */
+	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);
+
+		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);
+	} else
+		FAIL_TEST_IF_FALSE(ret2 == ptr);
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mmap_reuse_addr(bool seal)
+{
+	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);
+	}
+
+	/* use mmap to change protection. */
+	ret2 = mmap(ptr, size, PROT_NONE,
+			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+
+	/* MAP_FIXED is not used, expect new addr */
+	FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
+	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);
 
 	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

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.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@chromium.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
>

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
> On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.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@chromium.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.
>

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?

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

Correction - the middle is sealed. Other points remain.

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

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.

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

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> 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@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.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@chromium.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.
> >
>
> 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.

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

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

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

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@oracle.com> 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@chromium.org wrote:
> > > > From: Jeff Xu <jeffxu@chromium.org>
> > > >
> > > > 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@chromium.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.
> > >
> >
> > 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
> > > >

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Pedro Falcato]

On Sat, Sep 07, 2024 at 08:27:52PM GMT, Lorenzo Stoakes wrote:
> 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@oracle.com> 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@chromium.org wrote:
> > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > >
> > > > > 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.

Hi,

I agree with most of the points. Sitting down here to write unofficial
guidelines for mseal behavior.

mseal should seal regions and mark them immutable, which means their protection
and contents (ish?) (not _only_ backing mapping, but also contents in general
(see madvise below)) should not be changed throughout the lifetime of the address space.

For the general syscall interface, this means:
1) mprotect and munmap need to be blocked on mseal regions.
 1a) munmap _cannot_ tolerate partial failure, per POSIX.
 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.

2) Most madvise calls are allowed, except for destructive operations on
read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
about blocking these ops if we can do it manually with e.g memset)
 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
     different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
     stuff like madvise MADV_DONTNEED on program rodata.
 2b) We take into account pkeys when doing the permission checks.

3) mremap is not allowed as we'd change the "contents" of the old region.
 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
     We already informally allow expansion if e.g mmapping after it + mseal.

4) mlock and msync are allowed.

5) mseal is blocked.

6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.

7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
   should be disallowed (?)
 7a) This currently does not happen, and seems like a large hole? But disallowing this
     would probably severely break ptrace if the ELF sealing plans come to fruition.

When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
into a corner - this is strictly "undefined behavior". The msealed regions themselves
will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
also kind of a munmap-related test, and might not really be something we really want in the mseal tests)

Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
does not make sense to block operations unless they affect a VMA entirely, and that would also force
VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").


Do I have everything right? Am I missing anything? 

-- 
Pedro

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Pedro Falcato]

On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> Hi,
>
> I agree with most of the points. Sitting down here to write unofficial
> guidelines for mseal behavior.
>
> mseal should seal regions and mark them immutable, which means their protection
> and contents (ish?) (not _only_ backing mapping, but also contents in general
> (see madvise below)) should not be changed throughout the lifetime of the address space.
>
> For the general syscall interface, this means:
> 1) mprotect and munmap need to be blocked on mseal regions.
>  1a) munmap _cannot_ tolerate partial failure, per POSIX.
>  2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
>
> 2) Most madvise calls are allowed, except for destructive operations on
> read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
> about blocking these ops if we can do it manually with e.g memset)
>  2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
>      different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
>      stuff like madvise MADV_DONTNEED on program rodata.
>  2b) We take into account pkeys when doing the permission checks.
>
> 3) mremap is not allowed as we'd change the "contents" of the old region.
>  3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
>      We already informally allow expansion if e.g mmapping after it + mseal.
>
> 4) mlock and msync are allowed.
>
> 5) mseal is blocked.
>
> 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
>  6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
>
> 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
>    should be disallowed (?)
>  7a) This currently does not happen, and seems like a large hole? But disallowing this
>      would probably severely break ptrace if the ELF sealing plans come to fruition.
>
> When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
> means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
> of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
> into a corner - this is strictly "undefined behavior". The msealed regions themselves
> will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
> also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
>
> Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
> Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
> does not make sense to block operations unless they affect a VMA entirely, and that would also force
> VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
>
>
> Do I have everything right? Am I missing anything?

Small addendum: file write, truncate and hole punching might also
matter for sealed file-backed regions, as these change the region's
contents, but we probably
want to rely on file write permissions to protect against this (as we
already do). Any other solution is probably terrible and probably
endlessly NAK'd by fs folks, but it does
mean sealed regions aren't really immutable if you or the attacker can
write to the file.

-- 
Pedro

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Pedro

On Sun, Sep 8, 2024 at 2:56 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > Hi,
> >
> > I agree with most of the points. Sitting down here to write unofficial
> > guidelines for mseal behavior.
> >
> > mseal should seal regions and mark them immutable, which means their protection
> > and contents (ish?) (not _only_ backing mapping, but also contents in general
> > (see madvise below)) should not be changed throughout the lifetime of the address space.
> >
> > For the general syscall interface, this means:
> > 1) mprotect and munmap need to be blocked on mseal regions.
> >  1a) munmap _cannot_ tolerate partial failure, per POSIX.
> >  2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
> >
> > 2) Most madvise calls are allowed, except for destructive operations on
> > read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
> > about blocking these ops if we can do it manually with e.g memset)
> >  2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
> >      different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
> >      stuff like madvise MADV_DONTNEED on program rodata.
> >  2b) We take into account pkeys when doing the permission checks.
> >
> > 3) mremap is not allowed as we'd change the "contents" of the old region.
> >  3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
> >      We already informally allow expansion if e.g mmapping after it + mseal.
> >
> > 4) mlock and msync are allowed.
> >
> > 5) mseal is blocked.
> >
> > 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
> >  6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
> >
> > 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
> >    should be disallowed (?)
> >  7a) This currently does not happen, and seems like a large hole? But disallowing this
> >      would probably severely break ptrace if the ELF sealing plans come to fruition.
> >
> > When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
> > means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
> > of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
> > into a corner - this is strictly "undefined behavior". The msealed regions themselves
> > will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
> > also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
> >
> > Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
> > Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
> > does not make sense to block operations unless they affect a VMA entirely, and that would also force
> > VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
> >
> >
> > Do I have everything right? Am I missing anything?
>
> Small addendum: file write, truncate and hole punching might also
> matter for sealed file-backed regions, as these change the region's
> contents, but we probably
> want to rely on file write permissions to protect against this (as we
> already do). Any other solution is probably terrible and probably
> endlessly NAK'd by fs folks, but it does
> mean sealed regions aren't really immutable if you or the attacker can
> write to the file.
>
Right. The mseal protects the control-data of VMA (e.g. prot),
it doesn't do anything more than that. The file permission relies on
dac/mac control.


-Jeff


> --
> Pedro

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Pedro

On Sun, Sep 8, 2024 at 2:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> I agree with most of the points. Sitting down here to write unofficial
> guidelines for mseal behavior.
>
> mseal should seal regions and mark them immutable, which means their protection
> and contents (ish?) (not _only_ backing mapping, but also contents in general
> (see madvise below)) should not be changed throughout the lifetime of the address space.
>
> For the general syscall interface, this means:
> 1) mprotect and munmap need to be blocked on mseal regions.
>  1a) munmap _cannot_ tolerate partial failure, per POSIX.
>  2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
>
> 2) Most madvise calls are allowed, except for destructive operations on
> read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care
> about blocking these ops if we can do it manually with e.g memset)
>  2a) The current implementation only blocks discard on anonymous _regions_, which is slightly
>      different. We probably do want to block these on MAP_PRIVATE file mappings, as to block
>      stuff like madvise MADV_DONTNEED on program rodata.
>  2b) We take into account pkeys when doing the permission checks.
>
> 3) mremap is not allowed as we'd change the "contents" of the old region.
>  3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion.
>      We already informally allow expansion if e.g mmapping after it + mseal.
>
> 4) mlock and msync are allowed.
>
> 5) mseal is blocked.
mseal is not blocked, i.e. seal on an already sealed memory is
no-op. This is described in mseal.rst [1]

[1] https://github.com/torvalds/linux/blob/master/Documentation/userspace-api/mseal.rst

>
> 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
>  6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
>
> 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace)
>    should be disallowed (?)
>  7a) This currently does not happen, and seems like a large hole? But disallowing this
>      would probably severely break ptrace if the ELF sealing plans come to fruition.
>
Jann Horn  pointed out FOLL_FORCE during RFC [2], and this is in mseal.rst too.

In short, FOLL_FORCE is not covered by mseal. On ChromeOS, FOLL_FORCE
is disabled. Recently, Adrian Ratiu upstreamed that [3]

[2] https://lore.kernel.org/lkml/CAG48ez3ShUYey+ZAFsU2i1RpQn0a5eOs2hzQ426FkcgnfUGLvA@mail.gmail.com/
[3]   https://lore.kernel.org/lkml/20240802080225.89408-1-adrian.ratiu@collabora.com/

-Jeff

> When we say "disallowed", we usually (apart from munmap) allow for partial failure. This
> means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting
> of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves
> into a corner - this is strictly "undefined behavior". The msealed regions themselves
> will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is
> also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
>
> Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked.
> Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly
> does not make sense to block operations unless they affect a VMA entirely, and that would also force
> VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
>
>
> Do I have everything right? Am I missing anything?
>
> --
> Pedro

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Lorenzo

On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> 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@oracle.com> 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@chromium.org wrote:
> > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > >
> > > > > 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.
>
I'm also passionate about testing :-)

Maybe there is a mis-understanding ? I explained the purpose of this
patch set in various responses, maybe not directly to your email though.

Even though the number of lines is large in these patches, its main
intention is to test Pedro's in-place change (from can_modify_mm to
can_modify_vma). Before this patch,  the test had 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 were
feedback to me during in-loop change that selftest should be extensive
enough to discover all regressions.  Even though this viewpoint is subject
to debate. Since none would want to do it, I thought I would just do it.

So the Patch V3 1/5 is dedicated entirely to increasing the verification
for existing scenarios, this including checking return code code, vma-size,
etc after mm api return.

Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop
change, we discovered a bug in unmap(), and unmap() is not atomic.
This leads to 4/5(mmap), 5/5(mremap), which calls munmap().
In addition, I add scenarios to cover cross-multiple-vma cases.

The  high-level goal of mseal test are two folds:
1> make sure sealing is working correctly under different scenarios,
i.e. sealed mapping are not modified.
2> For unsealed memory, added mseal code  doesn't regress on regular mm API.

The goal 2 is as important as 1, that is why tests usually are done in
two phases, one with sealing, the other without.

> 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.
>
I could work on this (to split the patch further) if this helps
acceptance of the patch series.

However, since the merge window is closer, everyone is busy, and it
is not really urgent to get it merged.  the added tests  already
passed in the linux-next branch,  we could wait till after
merge-window to review/perfect those tests.

> 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.
>
My most test-infra related comments comes from Muhammad Usama Anjum
(added into this email), e.g. assert is not recommended.[1] ,

[1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/

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

Thanks
Hope a great weekend too !
-Jeff


-Jeff

> 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@chromium.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.
> > > >
> > >
> > > 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
> > > > >

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Fri, Sep 13, 2024 at 03:50:00PM -0700, Jeff Xu wrote:

> Even though the number of lines is large in these patches, its main
> intention is to test Pedro's in-place change (from can_modify_mm to
> can_modify_vma). Before this patch,  the test had 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 were
> feedback to me during in-loop change that selftest should be extensive
> enough to discover all regressions.  Even though this viewpoint is subject
> to debate. Since none would want to do it, I thought I would just do it.

> So the Patch V3 1/5 is dedicated entirely to increasing the verification
> for existing scenarios, this including checking return code code, vma-size,
> etc after mm api return.

> Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop
> change, we discovered a bug in unmap(), and unmap() is not atomic.
> This leads to 4/5(mmap), 5/5(mremap), which calls munmap().
> In addition, I add scenarios to cover cross-multiple-vma cases.

> The  high-level goal of mseal test are two folds:
> 1> make sure sealing is working correctly under different scenarios,
> i.e. sealed mapping are not modified.
> 2> For unsealed memory, added mseal code  doesn't regress on regular mm API.

> The goal 2 is as important as 1, that is why tests usually are done in
> two phases, one with sealing, the other without.

That's vastly more detail than is in the changelogs for the actual
patches (which are just a few lines each) or the cover letter of the
series.  I don't have the MM knowledge to assess the detail of what
you're saying but I can't help but think that it'd help a lot with
review if all this detail were part of the actual submission.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Mark

On Wed, Sep 18, 2024 at 6:18 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 13, 2024 at 03:50:00PM -0700, Jeff Xu wrote:
>
> > Even though the number of lines is large in these patches, its main
> > intention is to test Pedro's in-place change (from can_modify_mm to
> > can_modify_vma). Before this patch,  the test had 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 were
> > feedback to me during in-loop change that selftest should be extensive
> > enough to discover all regressions.  Even though this viewpoint is subject
> > to debate. Since none would want to do it, I thought I would just do it.
>
> > So the Patch V3 1/5 is dedicated entirely to increasing the verification
> > for existing scenarios, this including checking return code code, vma-size,
> > etc after mm api return.
>
> > Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop
> > change, we discovered a bug in unmap(), and unmap() is not atomic.
> > This leads to 4/5(mmap), 5/5(mremap), which calls munmap().
> > In addition, I add scenarios to cover cross-multiple-vma cases.
>
> > The  high-level goal of mseal test are two folds:
> > 1> make sure sealing is working correctly under different scenarios,
> > i.e. sealed mapping are not modified.
> > 2> For unsealed memory, added mseal code  doesn't regress on regular mm API.
>
> > The goal 2 is as important as 1, that is why tests usually are done in
> > two phases, one with sealing, the other without.
>
> That's vastly more detail than is in the changelogs for the actual
> patches (which are just a few lines each) or the cover letter of the
> series.  I don't have the MM knowledge to assess the detail of what
> you're saying but I can't help but think that it'd help a lot with
> review if all this detail were part of the actual submission.
Agreed, will update and give more detail in the next version of the patch.

Thanks
-Jeff

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Lorenzo and Muhammad

Reviving this thread since the merging window is closed and we have
more time to review /work on this code in the next few weeks.

On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
>
> Hi Lorenzo
>
> On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> >
> > 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.
> >
> My most test-infra related comments comes from Muhammad Usama Anjum
> (added into this email), e.g. assert is not recommended.[1] ,
>
> [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
>
Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE

Muhammad Usama Anjum doesn't want assert being used in selftest (see
[1] above), and I quote:
"We don't want to terminate the test if one test fails because of assert. We
want the sub-tests to get executed in-dependent of other tests.

ksft_test_result(condition, fmt, ...);
ksft_test_result_pass(fmt, ...);"

FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
replacement of assert.

Please let me know if you have questions on this and Muhammad might
also help to clarify the requirement if needed.

Thanks
-Jeff

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> Hi Lorenzo and Muhammad
>
> Reviving this thread since the merging window is closed and we have
> more time to review /work on this code in the next few weeks.
>
> On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
> >
> > Hi Lorenzo
> >
> > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > >
> > > 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.
> > >
> > My most test-infra related comments comes from Muhammad Usama Anjum
> > (added into this email), e.g. assert is not recommended.[1] ,
> >
> > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> >
> Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
>
> Muhammad Usama Anjum doesn't want assert being used in selftest (see
> [1] above), and I quote:
> "We don't want to terminate the test if one test fails because of assert. We
> want the sub-tests to get executed in-dependent of other tests.
>
> ksft_test_result(condition, fmt, ...);
> ksft_test_result_pass(fmt, ...);"
>
> FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> replacement of assert.
>
> Please let me know if you have questions on this and Muhammad might
> also help to clarify the requirement if needed.
>
> Thanks
> -Jeff

Right this is about not failing the test i.e. equivalent of an expect
rather than an assert, which makes sense.

What I'm saying is we should have something more like

EXPECT_TRUE()
EXPECT_FALSE()

etc.

Which would avoid these confusing

	FAIL_TEST_IF_FALSE(!expr)

Things.

Hopefully that's clear? Thanks!

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> > Hi Lorenzo and Muhammad
> >
> > Reviving this thread since the merging window is closed and we have
> > more time to review /work on this code in the next few weeks.
> >
> > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
> > >
> > > Hi Lorenzo
> > >
> > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > >
> > > > 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.
> > > >
> > > My most test-infra related comments comes from Muhammad Usama Anjum
> > > (added into this email), e.g. assert is not recommended.[1] ,
> > >
> > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> > >
> > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
> >
> > Muhammad Usama Anjum doesn't want assert being used in selftest (see
> > [1] above), and I quote:
> > "We don't want to terminate the test if one test fails because of assert. We
> > want the sub-tests to get executed in-dependent of other tests.
> >
> > ksft_test_result(condition, fmt, ...);
> > ksft_test_result_pass(fmt, ...);"
> >
> > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> > replacement of assert.
> >
> > Please let me know if you have questions on this and Muhammad might
> > also help to clarify the requirement if needed.
> >
> > Thanks
> > -Jeff
>
> Right this is about not failing the test i.e. equivalent of an expect
> rather than an assert, which makes sense.
>
> What I'm saying is we should have something more like
>
> EXPECT_TRUE()
> EXPECT_FALSE()
>
> etc.
>
> Which would avoid these confusing
>
>         FAIL_TEST_IF_FALSE(!expr)

FAIL_TEST_IF_FALSE(expr) is the right way to use this macro.

It is same syntax as assert(expr), e.g:

man assert(expr)
       assert - abort the program if assertion is false

FAIL_TEST_IF_FALSE is a replacement for assert,  instead of aborting
the program, it just reports failure in this test.

Is this still confusing ?
(The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax
of assert is well known.)


>
> Things.
>
> Hopefully that's clear? Thanks!

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote:
> On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> > > Hi Lorenzo and Muhammad
> > >
> > > Reviving this thread since the merging window is closed and we have
> > > more time to review /work on this code in the next few weeks.
> > >
> > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
> > > >
> > > > Hi Lorenzo
> > > >
> > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > >
> > > > >
> > > > > 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.
> > > > >
> > > > My most test-infra related comments comes from Muhammad Usama Anjum
> > > > (added into this email), e.g. assert is not recommended.[1] ,
> > > >
> > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> > > >
> > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
> > >
> > > Muhammad Usama Anjum doesn't want assert being used in selftest (see
> > > [1] above), and I quote:
> > > "We don't want to terminate the test if one test fails because of assert. We
> > > want the sub-tests to get executed in-dependent of other tests.
> > >
> > > ksft_test_result(condition, fmt, ...);
> > > ksft_test_result_pass(fmt, ...);"
> > >
> > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> > > replacement of assert.
> > >
> > > Please let me know if you have questions on this and Muhammad might
> > > also help to clarify the requirement if needed.
> > >
> > > Thanks
> > > -Jeff
> >
> > Right this is about not failing the test i.e. equivalent of an expect
> > rather than an assert, which makes sense.
> >
> > What I'm saying is we should have something more like
> >
> > EXPECT_TRUE()
> > EXPECT_FALSE()
> >
> > etc.
> >
> > Which would avoid these confusing
> >
> >         FAIL_TEST_IF_FALSE(!expr)
>
> FAIL_TEST_IF_FALSE(expr) is the right way to use this macro.

But you don't only test position conditions, you also test negative ones.

'Fail test if false false thing' is really confusing and hard to read.

I struggle to understand your tests as a result.

I understand 'fail test if false' is expressive in a way, but it's really hard
to parse.

Obviously it's also misleading in that you're saying 'fail the test _later_
if false', which I hadn't even realised...

It's well established in basically all normal test suites that:

* assert = fail test _here_ if this fails (actually a valid thing to do if
           you assert something that means the test simply cannot
           reasonably continue if that condition is false).
* expect = the test will now fail, but carry on.

I mean you work for a company that does this :) [0] this is a very well
established precedent.

[0]:https://github.com/google/googletest

>
> It is same syntax as assert(expr), e.g:
>
> man assert(expr)
>        assert - abort the program if assertion is false
>
> FAIL_TEST_IF_FALSE is a replacement for assert,  instead of aborting
> the program, it just reports failure in this test.

So doesn't at all do what assert does, because that _does_ terminate
execution on failure...

We are writing unit tests in a test framework, let's use very well
established industry practices please.

Also note that you don't even need to reinvent the wheel, there is a
fully-featured test harness available in
tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
EXPECT_xxx() helpers.

I've used it extensively myself and it works well.

I'd basically suggest you use that. Though moving existing tests to that
would be some churn.

On the other hand I really can't accept patches which are totally
unreadable to me, so you'll need to fix this one way or another, and the
churn is worth it as a one-time cost to be honest.

>
> Is this still confusing ?
> (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax
> of assert is well known.)

It's a super misleading name as it says nothing about _WHEN_ the test
fails. Also the syntax of assert() may be well known but you don't call
this function assert, you don't reference assert anywhere, and you don't do what
assert() does so, you know, That's not a great example.

The semantics of unit test frameworks are very well known, and already
implemented for you, and also do not require you to do unreadable double
negations for no reason, so let's use those please.

>
>
> >
> > Things.
> >
> > Hopefully that's clear? Thanks!

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

On Thu, Oct 17, 2024 at 12:00 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote:
> > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> > > > Hi Lorenzo and Muhammad
> > > >
> > > > Reviving this thread since the merging window is closed and we have
> > > > more time to review /work on this code in the next few weeks.
> > > >
> > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
> > > > >
> > > > > Hi Lorenzo
> > > > >
> > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > >
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > My most test-infra related comments comes from Muhammad Usama Anjum
> > > > > (added into this email), e.g. assert is not recommended.[1] ,
> > > > >
> > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> > > > >
> > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
> > > >
> > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see
> > > > [1] above), and I quote:
> > > > "We don't want to terminate the test if one test fails because of assert. We
> > > > want the sub-tests to get executed in-dependent of other tests.
> > > >
> > > > ksft_test_result(condition, fmt, ...);
> > > > ksft_test_result_pass(fmt, ...);"
> > > >
> > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> > > > replacement of assert.
> > > >
> > > > Please let me know if you have questions on this and Muhammad might
> > > > also help to clarify the requirement if needed.
> > > >
> > > > Thanks
> > > > -Jeff
> > >
> > > Right this is about not failing the test i.e. equivalent of an expect
> > > rather than an assert, which makes sense.
> > >
> > > What I'm saying is we should have something more like
> > >
> > > EXPECT_TRUE()
> > > EXPECT_FALSE()
> > >
> > > etc.
> > >
> > > Which would avoid these confusing
> > >
> > >         FAIL_TEST_IF_FALSE(!expr)
> >
> > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro.
>
> But you don't only test position conditions, you also test negative ones.
>
So it is not a problem with the MACRO, but where is it used ?

        ret = sys_mseal(ptr, size);
        FAIL_TEST_IF_FALSE(!ret);

Take this example, it would be
assert(!ret)

The syscall return usually returns 0 to indicate success, where a
negative comes from, but dev should be so used to (!ret), it is a
common pattern to check syscall return code. e.g assert(!ret)

Or do you have specific examples of code that caused confusion ?


> 'Fail test if false false thing' is really confusing and hard to read.
>
> I struggle to understand your tests as a result.
>
> I understand 'fail test if false' is expressive in a way, but it's really hard
> to parse.
>
If you just read it  as assert(), would that be easier ? (or you don't
like assert() ?)

> Obviously it's also misleading in that you're saying 'fail the test _later_
> if false', which I hadn't even realised...
>
> It's well established in basically all normal test suites that:
>
> * assert = fail test _here_ if this fails (actually a valid thing to do if
>            you assert something that means the test simply cannot
>            reasonably continue if that condition is false).
> * expect = the test will now fail, but carry on.
>
> I mean you work for a company that does this :) [0] this is a very well
> established precedent.
>
> [0]:https://github.com/google/googletest
>
Let's use expect as an example, let's say I create a new Macro:
TEST_EXPECT_TRUE, which basically is same syntax as
FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have
!ret in the code.

 ret = sys_mseal(ptr, size);
 TEST_EXPECT_TRUE(!ret);

Or is the FAIL_xxx_IF_FALSE pattern more un-readable than  EXPECT_TURE
? maybe ..

> >
> > It is same syntax as assert(expr), e.g:
> >
> > man assert(expr)
> >        assert - abort the program if assertion is false
> >
> > FAIL_TEST_IF_FALSE is a replacement for assert,  instead of aborting
> > the program, it just reports failure in this test.
>
> So doesn't at all do what assert does, because that _does_ terminate
> execution on failure...
>
I don't know what you mean, the test case will fail, but the next test
case will run. This the point, the mseal_test continues to run all
test cases to finish, even if one of the test cases is failed.

> We are writing unit tests in a test framework, let's use very well
> established industry practices please.
>
> Also note that you don't even need to reinvent the wheel, there is a
> fully-featured test harness available in
> tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> EXPECT_xxx() helpers.
>
The EXPECT_xxx() doesn't take care of reporting though,  or maybe it
needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't
spent much time on those, but on brief look, it seems it is for
repeating some tests, which doesn't exactly fit into what I needed,
e.g. the sealed memory won't be unmapped.
It is possible that those tests can be adopted to use test fixtures,
but I don't see significant gain for that.

> I've used it extensively myself and it works well.
>
> I'd basically suggest you use that. Though moving existing tests to that
> would be some churn.
>
> On the other hand I really can't accept patches which are totally
> unreadable to me, so you'll need to fix this one way or another, and the
> churn is worth it as a one-time cost to be honest.
>
> >
> > Is this still confusing ?
> > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax
> > of assert is well known.)
>
> It's a super misleading name as it says nothing about _WHEN_ the test
> fails. Also the syntax of assert() may be well known but you don't call
> this function assert, you don't reference assert anywhere, and you don't do what
> assert() does so, you know, That's not a great example.
>
> The semantics of unit test frameworks are very well known, and already
> implemented for you, and also do not require you to do unreadable double
> negations for no reason, so let's use those please.
>
As stated previously, I'm not sure whether the test fixture is
benefiting mseal_test at this moment.  But I'm open for future
conversion when I have time for this. For now, I like to get those
tests in so we can properly detect  possible regression for memory
sealing.

What will help you better review this code? Would the below help ?

s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g


> >
> >
> > >
> > > Things.
> > >
> > > Hopefully that's clear? Thanks!

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote:
> On Thu, Oct 17, 2024 at 12:00 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote:
> > > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> > > > > Hi Lorenzo and Muhammad
> > > > >
> > > > > Reviving this thread since the merging window is closed and we have
> > > > > more time to review /work on this code in the next few weeks.
> > > > >
> > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
> > > > > >
> > > > > > Hi Lorenzo
> > > > > >
> > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > My most test-infra related comments comes from Muhammad Usama Anjum
> > > > > > (added into this email), e.g. assert is not recommended.[1] ,
> > > > > >
> > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> > > > > >
> > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
> > > > >
> > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see
> > > > > [1] above), and I quote:
> > > > > "We don't want to terminate the test if one test fails because of assert. We
> > > > > want the sub-tests to get executed in-dependent of other tests.
> > > > >
> > > > > ksft_test_result(condition, fmt, ...);
> > > > > ksft_test_result_pass(fmt, ...);"
> > > > >
> > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> > > > > replacement of assert.
> > > > >
> > > > > Please let me know if you have questions on this and Muhammad might
> > > > > also help to clarify the requirement if needed.
> > > > >
> > > > > Thanks
> > > > > -Jeff
> > > >
> > > > Right this is about not failing the test i.e. equivalent of an expect
> > > > rather than an assert, which makes sense.
> > > >
> > > > What I'm saying is we should have something more like
> > > >
> > > > EXPECT_TRUE()
> > > > EXPECT_FALSE()
> > > >
> > > > etc.
> > > >
> > > > Which would avoid these confusing
> > > >
> > > >         FAIL_TEST_IF_FALSE(!expr)
> > >
> > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro.
> >
> > But you don't only test position conditions, you also test negative ones.
> >
> So it is not a problem with the MACRO, but where is it used ?
>
>         ret = sys_mseal(ptr, size);
>         FAIL_TEST_IF_FALSE(!ret);
>
> Take this example, it would be
> assert(!ret)
>
> The syscall return usually returns 0 to indicate success, where a
> negative comes from, but dev should be so used to (!ret), it is a
> common pattern to check syscall return code. e.g assert(!ret)
>
> Or do you have specific examples of code that caused confusion ?
>
>
> > 'Fail test if false false thing' is really confusing and hard to read.
> >
> > I struggle to understand your tests as a result.
> >
> > I understand 'fail test if false' is expressive in a way, but it's really hard
> > to parse.
> >
> If you just read it  as assert(), would that be easier ? (or you don't
> like assert() ?)
>
> > Obviously it's also misleading in that you're saying 'fail the test _later_
> > if false', which I hadn't even realised...
> >
> > It's well established in basically all normal test suites that:
> >
> > * assert = fail test _here_ if this fails (actually a valid thing to do if
> >            you assert something that means the test simply cannot
> >            reasonably continue if that condition is false).
> > * expect = the test will now fail, but carry on.
> >
> > I mean you work for a company that does this :) [0] this is a very well
> > established precedent.
> >
> > [0]:https://github.com/google/googletest
> >
> Let's use expect as an example, let's say I create a new Macro:
> TEST_EXPECT_TRUE, which basically is same syntax as
> FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have
> !ret in the code.
>
>  ret = sys_mseal(ptr, size);
>  TEST_EXPECT_TRUE(!ret);
>
> Or is the FAIL_xxx_IF_FALSE pattern more un-readable than  EXPECT_TURE
> ? maybe ..
>
> > >
> > > It is same syntax as assert(expr), e.g:
> > >
> > > man assert(expr)
> > >        assert - abort the program if assertion is false
> > >
> > > FAIL_TEST_IF_FALSE is a replacement for assert,  instead of aborting
> > > the program, it just reports failure in this test.
> >
> > So doesn't at all do what assert does, because that _does_ terminate
> > execution on failure...
> >
> I don't know what you mean, the test case will fail, but the next test
> case will run. This the point, the mseal_test continues to run all
> test cases to finish, even if one of the test cases is failed.
>
> > We are writing unit tests in a test framework, let's use very well
> > established industry practices please.
> >
> > Also note that you don't even need to reinvent the wheel, there is a
> > fully-featured test harness available in
> > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> > EXPECT_xxx() helpers.
> >
> The EXPECT_xxx() doesn't take care of reporting though,  or maybe it
> needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't
> spent much time on those, but on brief look, it seems it is for
> repeating some tests, which doesn't exactly fit into what I needed,
> e.g. the sealed memory won't be unmapped.
> It is possible that those tests can be adopted to use test fixtures,
> but I don't see significant gain for that.
>
> > I've used it extensively myself and it works well.
> >
> > I'd basically suggest you use that. Though moving existing tests to that
> > would be some churn.
> >
> > On the other hand I really can't accept patches which are totally
> > unreadable to me, so you'll need to fix this one way or another, and the
> > churn is worth it as a one-time cost to be honest.
> >
> > >
> > > Is this still confusing ?
> > > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax
> > > of assert is well known.)
> >
> > It's a super misleading name as it says nothing about _WHEN_ the test
> > fails. Also the syntax of assert() may be well known but you don't call
> > this function assert, you don't reference assert anywhere, and you don't do what
> > assert() does so, you know, That's not a great example.
> >
> > The semantics of unit test frameworks are very well known, and already
> > implemented for you, and also do not require you to do unreadable double
> > negations for no reason, so let's use those please.
> >
> As stated previously, I'm not sure whether the test fixture is
> benefiting mseal_test at this moment.  But I'm open for future
> conversion when I have time for this. For now, I like to get those
> tests in so we can properly detect  possible regression for memory
> sealing.
>
> What will help you better review this code? Would the below help ?
>
> s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g

Jeff, you're falling into your usual pattern of being unreasonably
argumentative for apparently no reason and I really don't have time to
constantly respond inline when you're just ignoring what I tell you.

You do this on nearly all code review and this just isn't working. If you
want to effectively be involved in mseal you need to LISTEN.

The more you do this the less patient everybody gets with you and the less
likely your series will ever get merged. This is not good for mseal or
anybody involved.

On this issue - either use sensible macros that YOU ARE DEFINING, not
assert.h, but YOU, that allow you to evaluate whether a condition is true
or false - or I will not accept your unreadable test code.

It's that simple and I'm done discussing this.

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

HHi Lorenzo

On Thu, Oct 17, 2024 at 11:38 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote:
> > On Thu, Oct 17, 2024 at 12:00 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote:
> > > > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes
> > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > >
> > > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote:
> > > > > > Hi Lorenzo and Muhammad
> > > > > >
> > > > > > Reviving this thread since the merging window is closed and we have
> > > > > > more time to review /work on this code in the next few weeks.
> > > > > >
> > > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Lorenzo
> > > > > > >
> > > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes
> > > > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > > My most test-infra related comments comes from Muhammad Usama Anjum
> > > > > > > (added into this email), e.g. assert is not recommended.[1] ,
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/
> > > > > > >
> > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE
> > > > > >
> > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see
> > > > > > [1] above), and I quote:
> > > > > > "We don't want to terminate the test if one test fails because of assert. We
> > > > > > want the sub-tests to get executed in-dependent of other tests.
> > > > > >
> > > > > > ksft_test_result(condition, fmt, ...);
> > > > > > ksft_test_result_pass(fmt, ...);"
> > > > > >
> > > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and
> > > > > > replacement of assert.
> > > > > >
> > > > > > Please let me know if you have questions on this and Muhammad might
> > > > > > also help to clarify the requirement if needed.
> > > > > >
> > > > > > Thanks
> > > > > > -Jeff
> > > > >
> > > > > Right this is about not failing the test i.e. equivalent of an expect
> > > > > rather than an assert, which makes sense.
> > > > >
> > > > > What I'm saying is we should have something more like
> > > > >
> > > > > EXPECT_TRUE()
> > > > > EXPECT_FALSE()
> > > > >
> > > > > etc.
> > > > >
> > > > > Which would avoid these confusing
> > > > >
> > > > >         FAIL_TEST_IF_FALSE(!expr)
> > > >
> > > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro.
> > >
> > > But you don't only test position conditions, you also test negative ones.
> > >
> > So it is not a problem with the MACRO, but where is it used ?
> >
> >         ret = sys_mseal(ptr, size);
> >         FAIL_TEST_IF_FALSE(!ret);
> >
> > Take this example, it would be
> > assert(!ret)
> >
> > The syscall return usually returns 0 to indicate success, where a
> > negative comes from, but dev should be so used to (!ret), it is a
> > common pattern to check syscall return code. e.g assert(!ret)
> >
> > Or do you have specific examples of code that caused confusion ?
> >
> >
> > > 'Fail test if false false thing' is really confusing and hard to read.
> > >
> > > I struggle to understand your tests as a result.
> > >
> > > I understand 'fail test if false' is expressive in a way, but it's really hard
> > > to parse.
> > >
> > If you just read it  as assert(), would that be easier ? (or you don't
> > like assert() ?)
> >
> > > Obviously it's also misleading in that you're saying 'fail the test _later_
> > > if false', which I hadn't even realised...
> > >
> > > It's well established in basically all normal test suites that:
> > >
> > > * assert = fail test _here_ if this fails (actually a valid thing to do if
> > >            you assert something that means the test simply cannot
> > >            reasonably continue if that condition is false).
> > > * expect = the test will now fail, but carry on.
> > >
> > > I mean you work for a company that does this :) [0] this is a very well
> > > established precedent.
> > >
> > > [0]:https://github.com/google/googletest
> > >
> > Let's use expect as an example, let's say I create a new Macro:
> > TEST_EXPECT_TRUE, which basically is same syntax as
> > FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have
> > !ret in the code.
> >
> >  ret = sys_mseal(ptr, size);
> >  TEST_EXPECT_TRUE(!ret);
> >
> > Or is the FAIL_xxx_IF_FALSE pattern more un-readable than  EXPECT_TURE
> > ? maybe ..
> >
> > > >
> > > > It is same syntax as assert(expr), e.g:
> > > >
> > > > man assert(expr)
> > > >        assert - abort the program if assertion is false
> > > >
> > > > FAIL_TEST_IF_FALSE is a replacement for assert,  instead of aborting
> > > > the program, it just reports failure in this test.
> > >
> > > So doesn't at all do what assert does, because that _does_ terminate
> > > execution on failure...
> > >
> > I don't know what you mean, the test case will fail, but the next test
> > case will run. This the point, the mseal_test continues to run all
> > test cases to finish, even if one of the test cases is failed.
> >
> > > We are writing unit tests in a test framework, let's use very well
> > > established industry practices please.
> > >
> > > Also note that you don't even need to reinvent the wheel, there is a
> > > fully-featured test harness available in
> > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> > > EXPECT_xxx() helpers.
> > >
> > The EXPECT_xxx() doesn't take care of reporting though,  or maybe it
> > needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't
> > spent much time on those, but on brief look, it seems it is for
> > repeating some tests, which doesn't exactly fit into what I needed,
> > e.g. the sealed memory won't be unmapped.
> > It is possible that those tests can be adopted to use test fixtures,
> > but I don't see significant gain for that.
> >
> > > I've used it extensively myself and it works well.
> > >
> > > I'd basically suggest you use that. Though moving existing tests to that
> > > would be some churn.
> > >
> > > On the other hand I really can't accept patches which are totally
> > > unreadable to me, so you'll need to fix this one way or another, and the
> > > churn is worth it as a one-time cost to be honest.
> > >
> > > >
> > > > Is this still confusing ?
> > > > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax
> > > > of assert is well known.)
> > >
> > > It's a super misleading name as it says nothing about _WHEN_ the test
> > > fails. Also the syntax of assert() may be well known but you don't call
> > > this function assert, you don't reference assert anywhere, and you don't do what
> > > assert() does so, you know, That's not a great example.
> > >
> > > The semantics of unit test frameworks are very well known, and already
> > > implemented for you, and also do not require you to do unreadable double
> > > negations for no reason, so let's use those please.
> > >
> > As stated previously, I'm not sure whether the test fixture is
> > benefiting mseal_test at this moment.  But I'm open for future
> > conversion when I have time for this. For now, I like to get those
> > tests in so we can properly detect  possible regression for memory
> > sealing.
> >
> > What will help you better review this code? Would the below help ?
> >
> > s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g
>
> Jeff, you're falling into your usual pattern of being unreasonably
> argumentative for apparently no reason and I really don't have time to
> constantly respond inline when you're just ignoring what I tell you.
>
> You do this on nearly all code review and this just isn't working. If you
> want to effectively be involved in mseal you need to LISTEN.
>
> The more you do this the less patient everybody gets with you and the less
> likely your series will ever get merged. This is not good for mseal or
> anybody involved.
>
> On this issue - either use sensible macros that YOU ARE DEFINING, not
> assert.h, but YOU, that allow you to evaluate whether a condition is true
> or false - or I will not accept your unreadable test code.
>
> It's that simple and I'm done discussing this.

Thanks for your time on discussing this.

Please, if I may say, when presenting your argument, keep it technical
and avoid personal attack. Using personal attacks rather than using
logic to refute your argument is “Ad Hominem Fallacy” [1]  and will
make it harder to get your point across.

[1] https://www.txst.edu/philosophy/resources/fallacy-definitions/ad-hominem.html#:~:text=(Attacking%20the%20person)%3A%20This,who%20is%20making%20the%20argument.

Additionally, The mseal_test was reviewed-by Muhammad Usama Anjum
during original RFC discussion. IIUC, Muhammad Usama Anjum has domain
knowledge for selftest infra, and I have relied on Muhammad’s comments
and implemented all those comments.

I'm not saying there is no room for improvement, but it should happen
in more respectful and constructive ways. In any case, I hope we have
common interest  and passion to  get more test coverage to avoid
future regression.  Given that we had 2 regressions in the past during
code reviews and a pending regression to fix at this moment in memory
sealing area,  the benefit of additional test coverage is obvious.

Specific on FAIL_TEST_IF_FALS macro, during the course of this
discussion, your comments have started with, and I quote:

“ 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().”

“This is beyond horrible. You really have to add more asserts.”

TO my response:

“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 counting how many tests are
failing.”

And your question:
“why we have to assert negations only, and other self tests do not do this.”

And my response:
"My most test-infra related comments comes from Muhammad Usama Anjum"
(added into this email), e.g. assert is not recommended.[1] ,
[1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/"

And my additional  try to clarify about your question about negations:
“So it is not a problem with the MACRO, but where is it used ?
        ret = sys_mseal(ptr, size);
        FAIL_TEST_IF_FALSE(!ret);
Take this example, it would be
assert(!ret)
The syscall return usually returns 0 to indicate success, where a
negative comes from, but dev should be so used to (!ret), it is a
common pattern to check syscall return code. e.g assert(!ret)"

And I offer an alternative approach for macro naming:
"ret = sys_mseal(ptr, size);
TEST_EXPECT_TRUE(!ret);"

Which you didn’t respond to directly.

Given the situation, I think it might be best to let domain experts
from the testinfra team, such as Muhammad to suggestion a better
replacement for this macro.

Best regards,
-Jeff

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Fri, Oct 18, 2024 at 11:01:40AM -0700, Jeff Xu wrote:
> >
> > Jeff, you're falling into your usual pattern of being unreasonably
> > argumentative for apparently no reason and I really don't have time to
> > constantly respond inline when you're just ignoring what I tell you.
> >
> > You do this on nearly all code review and this just isn't working. If you
> > want to effectively be involved in mseal you need to LISTEN.
> >
> > The more you do this the less patient everybody gets with you and the less
> > likely your series will ever get merged. This is not good for mseal or
> > anybody involved.
> >
> > On this issue - either use sensible macros that YOU ARE DEFINING, not
> > assert.h, but YOU, that allow you to evaluate whether a condition is true
> > or false - or I will not accept your unreadable test code.
> >
> > It's that simple and I'm done discussing this.
>
> Thanks for your time on discussing this.
>
> Please, if I may say, when presenting your argument, keep it technical
> and avoid personal attack. Using personal attacks rather than using
> logic to refute your argument is “Ad Hominem Fallacy” [1]  and will
> make it harder to get your point across.

This is not how ad hominem works, Jeff, it's a common misunderstanding of
what that means.

It'd be the case if in replying to a specific technical point I was to
respond with something personal. That is not what is happening here.

In fact it is not personal at all - code review consists of 1. technical
points and 2. how well the conversation goes on review. If 2 is absolutely
failing, it is absolutely fair and pertinent to bring that up.

What is happening here is that we have spent several months trying to
explain to you very very simple points:

1. Have macros that assert both truth and falsity so you don't have some
   inverted and unreadable assert.

2. Do not use arbitrary 'magic numbers'.

Instead of listening, you have been needlessly difficult in a way others
are not. I can't spend chunks of my working day going point-by-point
knowing you will ultimately say something like 'well I just don't want to'
or simply ignore points.

It is a waste of both of our time, but it is what you do.

>
> [1] https://www.txst.edu/philosophy/resources/fallacy-definitions/ad-hominem.html#:~:text=(Attacking%20the%20person)%3A%20This,who%20is%20making%20the%20argument.
>
> Additionally, The mseal_test was reviewed-by Muhammad Usama Anjum
> during original RFC discussion. IIUC, Muhammad Usama Anjum has domain
> knowledge for selftest infra, and I have relied on Muhammad’s comments
> and implemented all those comments.

If I can't read the tests, I'm going to NACK the series, sorry.

>
> I'm not saying there is no room for improvement, but it should happen
> in more respectful and constructive ways. In any case, I hope we have
> common interest  and passion to  get more test coverage to avoid
> future regression.  Given that we had 2 regressions in the past during
> code reviews and a pending regression to fix at this moment in memory
> sealing area,  the benefit of additional test coverage is obvious.

You are repeatedly ignoring what you are being told by me and others. You
have done this consistently to the point that you are taking up quite a bit
of our time.

You do this on pretty much all code reviews. There is not just 'room for
improvement', you are bordering on being impossible to deal with.

The benefits of additional test coverage is indeed obvious, but not if the
tests are largely redundant, misleading or confused, which yours in fact
are.

This is why I proposed that we sit down and figure out exactly what it is
you want to test ahead of time, and NACKed the series (I'm not even quite
sure why we are discussing this here still).

The fact you're debating about using ASSERT(), EXPECT() on the same series
months later is not encouraging.

>
> Specific on FAIL_TEST_IF_FALS macro, during the course of this
> discussion, your comments have started with, and I quote:
>
> “ 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().”
>
> “This is beyond horrible. You really have to add more asserts.”
>
> TO my response:
>
> “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 counting how many tests are
> failing.”
>
> And your question:
> “why we have to assert negations only, and other self tests do not do this.”
>
> And my response:
> "My most test-infra related comments comes from Muhammad Usama Anjum"
> (added into this email), e.g. assert is not recommended.[1] ,
> [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/"

This is a PERFECT example of the problem.

You are excluding my response to this - that he said NO SUCH THING - but
rather he was talking about having an EXPECT()-like pattern!

You repeatedly do this - you ignore responses that contradict you. This is
not a functional way to do code review.

>
> And my additional  try to clarify about your question about negations:
> “So it is not a problem with the MACRO, but where is it used ?
>         ret = sys_mseal(ptr, size);
>         FAIL_TEST_IF_FALSE(!ret);
> Take this example, it would be
> assert(!ret)
> The syscall return usually returns 0 to indicate success, where a
> negative comes from, but dev should be so used to (!ret), it is a
> common pattern to check syscall return code. e.g assert(!ret)"
>
> And I offer an alternative approach for macro naming:
> "ret = sys_mseal(ptr, size);
> TEST_EXPECT_TRUE(!ret);"
>
> Which you didn’t respond to directly.

Because we are going round in circles on 2 very very simple points and I
don't have infinite time.

Simply introduce ASSERT_TRUE(), ASSERT_FALSE(), EXPECT_TRUE(),
EXPECT_FALSE() perhaps with different spellings (no idea why you would want
to do that) or use the test harness.

Instead you try to debate 'oh this is kind of like assert' or implying
somebody said something or etc. etc. over dozens of emails.

>
> Given the situation, I think it might be best to let domain experts
> from the testinfra team, such as Muhammad to suggestion a better
> replacement for this macro.

I'm not sure what kind of 'domain expertise' is required for you to not use
magic numbers and to not create unreadable expressions. This is basic stuff.

The 'situation' is that I've asked you for 2 extremely simple things that
pretty well all other tests do and you are arguing about it 2 months later
on a NACKed series.

Inviting others into the thread is not going to help that.

>
> Best regards,
> -Jeff

As you know I've gone out of my way and dedicated quite a bit of time to
trying to find various different solutions to this.

It is not far off 10pm here and I am sat here replying to this. I would
please ask you to take a step back, stop being so defensive, and just
listen.

Go read some other code reviews in mm and see how successful code reviews
progress. Concede some points. Defer to domain expertise and to maintainers
at least on occasion.

The reason I keep on bringing up what you characterise as 'ad hominem'
(incorrectly, though it's a very common misunderstanding of what that
means), is because the problem in these reviews isn't technical - it is
your unwillingness to listen and engage with reviewers.

If that isn't fixed, I don't see how any of your series can progress.

And in general I don't think I can sink more time into giving as detailed
feedback on the review process as this. I have tried to do what I can here.

Anyway, I hope that we can find a positive resolution to this as I've said
before.

Thanks, Lorenzo

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2929 bytes --]

On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote:

> So it is not a problem with the MACRO, but where is it used ?

>         ret = sys_mseal(ptr, size);
>         FAIL_TEST_IF_FALSE(!ret);

> Take this example, it would be
> assert(!ret)

The problem is that the macro name is confusing and not terribly
consistent with how the rest of the selftests work.  The standard
kselftest result reporting is

	ksft_test_result(bool result, char *name_format, ...);

so the result of the test is a boolean flag which is passed in.  This
macro on the other hand sounds like a double negative so you have to
stop and think what the logic is, and it's not seen anywhere else so
nobody is going to be familiar with it.  The main thing this is doing is
burying a return statement in there, that's a bit surprising too.

I'll also note that these macros are resulting in broken kselftest
output, the name for a test has to be stable for automated systems to be
able to associate test results between runs but these print

                        ksft_test_result_fail("%s: line:%d\n",          \
                                                __func__, __LINE__);    \
                        return;                                         \

which includes the line number of the test in the name which is an
obvious problem, automated systems won't be able to tell that any two
failures are related to each other never mind the passing test.  We
should report why things failed but it's better to do that with a
ksft_print_msg(), ideally one that's directly readable rather than
requiring someone to go into the source code and look it up.

A more standard way to write what you've got here would be to have the
tests return a bool then have a runner loop which iterates over the
tests:

	struct {
		char *name;
		bool (*func)(void);
	} tests[];

	...

	for (i = 0; i < ARRAY_SIZE(tests); i++)
		ksft_test_result(tests[i].test(), tests[i].name);

then the tests can just have explicit return statements and don't need
to worry about logging anything other than diagnostics.

Depending on how much you need to share between tests you might also be
able to use kselftest_harness.h which fork()s each test into a separate
child and allows you to just fault to fail if that's easier.

> > We are writing unit tests in a test framework, let's use very well
> > established industry practices please.

Plus also the fact that we have a framework here...

> > Also note that you don't even need to reinvent the wheel, there is a
> > fully-featured test harness available in
> > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> > EXPECT_xxx() helpers.

> The EXPECT_xxx() doesn't take care of reporting though,  or maybe it

I rather think people would've noticed if the test harness was so broken
that it was unable to report failures.  If it is that broken we should
fix it rather than open coding something else.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Mark and Muhammad

On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote:
>
> > So it is not a problem with the MACRO, but where is it used ?
>
> >         ret = sys_mseal(ptr, size);
> >         FAIL_TEST_IF_FALSE(!ret);
>
> > Take this example, it would be
> > assert(!ret)
>
> The problem is that the macro name is confusing and not terribly
> consistent with how the rest of the selftests work.  The standard
> kselftest result reporting is
>
>         ksft_test_result(bool result, char *name_format, ...);
>
> so the result of the test is a boolean flag which is passed in.  This
> macro on the other hand sounds like a double negative so you have to
> stop and think what the logic is, and it's not seen anywhere else so
> nobody is going to be familiar with it.  The main thing this is doing is
> burying a return statement in there, that's a bit surprising too.
>
Thanks for explaining the problem, naming is hard. Do you have a
suggestion on a better naming?

> I'll also note that these macros are resulting in broken kselftest
> output, the name for a test has to be stable for automated systems to be
> able to associate test results between runs but these print
>
>                         ksft_test_result_fail("%s: line:%d\n",          \
>                                                 __func__, __LINE__);    \
>                         return;                                         \
>
> which includes the line number of the test in the name which is an
> obvious problem, automated systems won't be able to tell that any two
> failures are related to each other never mind the passing test.  We
> should report why things failed but it's better to do that with a
> ksft_print_msg(), ideally one that's directly readable rather than
> requiring someone to go into the source code and look it up.
>
I don't know what  the issue you described is ? Are you saying that we
are missing line numbers ? it is not. here is the sample of output:
Failure in the second test case from last:

ok 105 test_munmap_free_multiple_ranges
not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573
ok 107 test_munmap_free_multiple_ranges_with_split
# Planned tests != run tests (106 != 107)

> A more standard way to write what you've got here would be to have the
> tests return a bool then have a runner loop which iterates over the
> tests:
>
>         struct {
>                 char *name;
>                 bool (*func)(void);
>         } tests[];
>
>         ...
>
>         for (i = 0; i < ARRAY_SIZE(tests); i++)
>                 ksft_test_result(tests[i].test(), tests[i].name);
>
> then the tests can just have explicit return statements and don't need
> to worry about logging anything other than diagnostics.
>
> Depending on how much you need to share between tests you might also be
> able to use kselftest_harness.h which fork()s each test into a separate
> child and allows you to just fault to fail if that's easier.
>
> > > We are writing unit tests in a test framework, let's use very well
> > > established industry practices please.
>
> Plus also the fact that we have a framework here...
>
> > > Also note that you don't even need to reinvent the wheel, there is a
> > > fully-featured test harness available in
> > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> > > EXPECT_xxx() helpers.
>
> > The EXPECT_xxx() doesn't take care of reporting though,  or maybe it
>
> I rather think people would've noticed if the test harness was so broken
> that it was unable to report failures.  If it is that broken we should
> fix it rather than open coding something else.

In general, I agree with those comments, but I would like to rely on
domain experts in test infra to recommend what to use, or is
acceptable.

In this case, I hope Muhammad, who reviewed this code in the first
place, can make recommendations on a replacement of this macro.

I would image the needs of something similar to FAIL_TEST_IF_FALSE is
common in selftest writing:

1> lightweight enough so dev can pick up quickly and adapt to existing
tests, instead of rewriting everything from scratch.
2> assert like syntax
3> fail the current test case, but continue running the next test case
4> take care of reporting test failures.

Thanks
-Jeff

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3673 bytes --]

On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:
> On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote:

> > The problem is that the macro name is confusing and not terribly
> > consistent with how the rest of the selftests work.  The standard
> > kselftest result reporting is

> >         ksft_test_result(bool result, char *name_format, ...);
> >
> > so the result of the test is a boolean flag which is passed in.  This
> > macro on the other hand sounds like a double negative so you have to
> > stop and think what the logic is, and it's not seen anywhere else so
> > nobody is going to be familiar with it.  The main thing this is doing is
> > burying a return statement in there, that's a bit surprising too.

> Thanks for explaining the problem, naming is hard. Do you have a
> suggestion on a better naming?

Honestly I'd probably deal with this by refactoring such that the macro
isn't needed and the tests follow a pattern more like:

	if (ret != 0) {
		ksft_print_msg("$ACTION failed with %d\n", ret);
		return false;
	}

when they encouter a failure, the pattern I sketched in my earlier
message, or switch to kselftest_harness.h (like I say I don't know if
the fork()ing is an issue for these tests).  If I had to have a macro
it'd probably be something like mseal_assert().

> > I'll also note that these macros are resulting in broken kselftest
> > output, the name for a test has to be stable for automated systems to be
> > able to associate test results between runs but these print

....

> > which includes the line number of the test in the name which is an
> > obvious problem, automated systems won't be able to tell that any two
> > failures are related to each other never mind the passing test.  We
> > should report why things failed but it's better to do that with a
> > ksft_print_msg(), ideally one that's directly readable rather than
> > requiring someone to go into the source code and look it up.

> I don't know what  the issue you described is ? Are you saying that we
> are missing line numbers ? it is not. here is the sample of output:

No, I'm saying that having the line numbers is a problem.

> Failure in the second test case from last:

> ok 105 test_munmap_free_multiple_ranges
> not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573
> ok 107 test_munmap_free_multiple_ranges_with_split

Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
line:2573" which automated systems aren't going to be able to associate
with the passing "test_munmap_free_multiple_ranges_with_split", nor with
any failures that occur on any other lines in the function.

> I would image the needs of something similar to FAIL_TEST_IF_FALSE is
> common in selftest writing:

> 1> lightweight enough so dev can pick up quickly and adapt to existing
> tests, instead of rewriting everything from scratch.
> 2> assert like syntax
> 3> fail the current test case, but continue running the next test case
> 4> take care of reporting test failures.

Honestly this just sounds and looks like kselftest_harness.h, it's
ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
for asserts.  The main gotchas with it are that it's not particularly
elegant for test cases which need to enumerate system features (which I
don't think is the case for mseal()?) and that it runs each test case in
a fork()ed child which can be inconvenient for some tests.  The use of
fork() is because that make the overall test program much more robust
against breakage in individual tests and allows things like per test
timeouts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

Hi Mark

On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:
> > On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > The problem is that the macro name is confusing and not terribly
> > > consistent with how the rest of the selftests work.  The standard
> > > kselftest result reporting is
>
> > >         ksft_test_result(bool result, char *name_format, ...);
> > >
> > > so the result of the test is a boolean flag which is passed in.  This
> > > macro on the other hand sounds like a double negative so you have to
> > > stop and think what the logic is, and it's not seen anywhere else so
> > > nobody is going to be familiar with it.  The main thing this is doing is
> > > burying a return statement in there, that's a bit surprising too.
>
> > Thanks for explaining the problem, naming is hard. Do you have a
> > suggestion on a better naming?
>
> Honestly I'd probably deal with this by refactoring such that the macro
> isn't needed and the tests follow a pattern more like:
>
>         if (ret != 0) {
>                 ksft_print_msg("$ACTION failed with %d\n", ret);
>                 return false;
>         }
>
So expanding the macro to actually code ?
But this makes the meal_test  quite large with lots of "if", and I
would rather avoid that.


> when they encouter a failure, the pattern I sketched in my earlier
> message, or switch to kselftest_harness.h (like I say I don't know if
> the fork()ing is an issue for these tests).  If I had to have a macro
> it'd probably be something like mseal_assert().
>
I can go with mseal_assert, the original macro is used  by mseal_test
itself, and only intended as such.

If changing name to mseal_assert() is acceptable, this seems to be a
minimum change and I'm happy with that.

> > > I'll also note that these macros are resulting in broken kselftest
> > > output, the name for a test has to be stable for automated systems to be
> > > able to associate test results between runs but these print
>
> ....
>
> > > which includes the line number of the test in the name which is an
> > > obvious problem, automated systems won't be able to tell that any two
> > > failures are related to each other never mind the passing test.  We
> > > should report why things failed but it's better to do that with a
> > > ksft_print_msg(), ideally one that's directly readable rather than
> > > requiring someone to go into the source code and look it up.
>
> > I don't know what  the issue you described is ? Are you saying that we
> > are missing line numbers ? it is not. here is the sample of output:
>
> No, I'm saying that having the line numbers is a problem.
>
> > Failure in the second test case from last:
>
> > ok 105 test_munmap_free_multiple_ranges
> > not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573
> > ok 107 test_munmap_free_multiple_ranges_with_split
>
> Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
> line:2573" which automated systems aren't going to be able to associate
> with the passing "test_munmap_free_multiple_ranges_with_split", nor with
> any failures that occur on any other lines in the function.
>
I see. That will happen when those tests are modified and line number
changes. I could see reasoning for this argument, especially when
those tests are flaky and get updated often.

In practice, I hope any of those kernel self-test failures should get
fixed immediately, or even better, run before dev submitting the patch
that affects the mm area.

Having line number does help dev to go to error directly, and I'm not
against filling in the "action" field, but you might also agree with
me, finding unique text for each error would require some decent
amount of time, especially for large tests such as mseal_test.

> > I would image the needs of something similar to FAIL_TEST_IF_FALSE is
> > common in selftest writing:
>
> > 1> lightweight enough so dev can pick up quickly and adapt to existing
> > tests, instead of rewriting everything from scratch.
> > 2> assert like syntax
> > 3> fail the current test case, but continue running the next test case
> > 4> take care of reporting test failures.
>
> Honestly this just sounds and looks like kselftest_harness.h, it's
> ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
> for asserts.  The main gotchas with it are that it's not particularly
> elegant for test cases which need to enumerate system features (which I
> don't think is the case for mseal()?) and that it runs each test case in
> a fork()ed child which can be inconvenient for some tests.  The use of
> fork() is because that makes the overall test program much more robust
> against breakage in individual tests and allows things like per test
> timeouts.
OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture.

If I  switch to test_fixture, e,g, using TEST(test_name)

how do I pass the "seal" flag to it ?
e.g. how do I run the same test twice, first seal = true, and second seal=false.

        test_seal_mmap_shrink(false);
        test_seal_mmap_shrink(true);

The example [1], isn't clear about that.

https://www.kernel.org/doc/html/v4.19/dev-tools/kselftest.html#example

Thanks

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Lorenzo Stoakes]

On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote:
> > when they encouter a failure, the pattern I sketched in my earlier
> > message, or switch to kselftest_harness.h (like I say I don't know if
> > the fork()ing is an issue for these tests).  If I had to have a macro
> > it'd probably be something like mseal_assert().
> >
> I can go with mseal_assert, the original macro is used  by mseal_test
> itself, and only intended as such.
>
> If changing name to mseal_assert() is acceptable, this seems to be a
> minimum change and I'm happy with that.

No.

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Shuah Khan]

On 10/18/24 13:52, Lorenzo Stoakes wrote:
> On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote:
>>> when they encouter a failure, the pattern I sketched in my earlier
>>> message, or switch to kselftest_harness.h (like I say I don't know if
>>> the fork()ing is an issue for these tests).  If I had to have a macro
>>> it'd probably be something like mseal_assert().
>>>
>> I can go with mseal_assert, the original macro is used  by mseal_test
>> itself, and only intended as such.
>>
>> If changing name to mseal_assert() is acceptable, this seems to be a
>> minimum change and I'm happy with that.
> 
> No.
> 

Jeff,

Please pay attention to the feedback you have been receiving so far from
Mark and others about using the existing kselftest framework for reporting
results and don't reinvent wheel.

We have two frameworks to choose from - they both have been in use for
quiet sometime by tests. If there is a need to add new functions and
fix the existing ones that should happen in kselftest_harness.h or
kselftest.h. We keep fixing problem and enhancing them as needed.

With my kselfest and framework maintainer hat on, I don't want to see
yet another framework emerging which is buried in tests.

thanks,
-- Shuah

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Shuah Khan]

On 10/18/24 13:32, Jeff Xu wrote:
> Hi Mark
> 
> On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote:
>>
>> On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:
>>> On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote:
>>
>>>> The problem is that the macro name is confusing and not terribly
>>>> consistent with how the rest of the selftests work.  The standard
>>>> kselftest result reporting is
>>
>>>>          ksft_test_result(bool result, char *name_format, ...);
>>>>
>>>> so the result of the test is a boolean flag which is passed in.  This
>>>> macro on the other hand sounds like a double negative so you have to
>>>> stop and think what the logic is, and it's not seen anywhere else so
>>>> nobody is going to be familiar with it.  The main thing this is doing is
>>>> burying a return statement in there, that's a bit surprising too.
>>
>>> Thanks for explaining the problem, naming is hard. Do you have a
>>> suggestion on a better naming?
>>
>> Honestly I'd probably deal with this by refactoring such that the macro
>> isn't needed and the tests follow a pattern more like:
>>
>>          if (ret != 0) {
>>                  ksft_print_msg("$ACTION failed with %d\n", ret);
>>                  return false;
>>          }
>>
> So expanding the macro to actually code ?
> But this makes the meal_test  quite large with lots of "if", and I
> would rather avoid that.
> 
> 
>> when they encouter a failure, the pattern I sketched in my earlier
>> message, or switch to kselftest_harness.h (like I say I don't know if
>> the fork()ing is an issue for these tests).  If I had to have a macro
>> it'd probably be something like mseal_assert().
>>
> I can go with mseal_assert, the original macro is used  by mseal_test
> itself, and only intended as such.
> 
> If changing name to mseal_assert() is acceptable, this seems to be a
> minimum change and I'm happy with that.
> 
>>>> I'll also note that these macros are resulting in broken kselftest
>>>> output, the name for a test has to be stable for automated systems to be
>>>> able to associate test results between runs but these print
>>
>> ....
>>
>>>> which includes the line number of the test in the name which is an
>>>> obvious problem, automated systems won't be able to tell that any two
>>>> failures are related to each other never mind the passing test.  We
>>>> should report why things failed but it's better to do that with a
>>>> ksft_print_msg(), ideally one that's directly readable rather than
>>>> requiring someone to go into the source code and look it up.
>>
>>> I don't know what  the issue you described is ? Are you saying that we
>>> are missing line numbers ? it is not. here is the sample of output:
>>
>> No, I'm saying that having the line numbers is a problem.
>>
>>> Failure in the second test case from last:
>>
>>> ok 105 test_munmap_free_multiple_ranges
>>> not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573
>>> ok 107 test_munmap_free_multiple_ranges_with_split
>>
>> Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
>> line:2573" which automated systems aren't going to be able to associate
>> with the passing "test_munmap_free_multiple_ranges_with_split", nor with
>> any failures that occur on any other lines in the function.
>>
> I see. That will happen when those tests are modified and line number
> changes. I could see reasoning for this argument, especially when
> those tests are flaky and get updated often.
> 
> In practice, I hope any of those kernel self-test failures should get
> fixed immediately, or even better, run before dev submitting the patch
> that affects the mm area.
> 
> Having line number does help dev to go to error directly, and I'm not
> against filling in the "action" field, but you might also agree with
> me, finding unique text for each error would require some decent
> amount of time, especially for large tests such as mseal_test.
> 
>>> I would image the needs of something similar to FAIL_TEST_IF_FALSE is
>>> common in selftest writing:
>>
>>> 1> lightweight enough so dev can pick up quickly and adapt to existing
>>> tests, instead of rewriting everything from scratch.
>>> 2> assert like syntax
>>> 3> fail the current test case, but continue running the next test case
>>> 4> take care of reporting test failures.
>>
>> Honestly this just sounds and looks like kselftest_harness.h, it's
>> ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
>> for asserts.  The main gotchas with it are that it's not particularly
>> elegant for test cases which need to enumerate system features (which I
>> don't think is the case for mseal()?) and that it runs each test case in
>> a fork()ed child which can be inconvenient for some tests.  The use of
>> fork() is because that makes the overall test program much more robust
>> against breakage in individual tests and allows things like per test
>> timeouts.
> OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture.
> 
> If I  switch to test_fixture, e,g, using TEST(test_name)
> 
> how do I pass the "seal" flag to it ?

Doesn't TH_LOG() work for you to pass arguments?

> e.g. how do I run the same test twice, first seal = true, and second seal=false.
> 
>          test_seal_mmap_shrink(false);
>          test_seal_mmap_shrink(true);
> 
> The example [1], isn't clear about that.
> 
> https://www.kernel.org/doc/html/v4.19/dev-tools/kselftest.html#example
> 
> Thanks
> 

thanks,
-- Shuah

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4678 bytes --]

On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote:
> On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:

> > Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
> > line:2573" which automated systems aren't going to be able to associate
> > with the passing "test_munmap_free_multiple_ranges_with_split", nor with
> > any failures that occur on any other lines in the function.

> I see. That will happen when those tests are modified and line number
> changes. I could see reasoning for this argument, especially when
> those tests are flaky and get updated often.

> In practice, I hope any of those kernel self-test failures should get
> fixed immediately, or even better, run before dev submitting the patch
> that affects the mm area.

That's not the entire issue - it is also a problem that the test name
is not the same between passes and failures so automated systems can't
associate the failures with the passes.  When a test starts failing they
will see the passing test disappear and a new test appear that has never
worked.  This will mean that for example if they have bisection support
or UI for showing when a test started regressing those won't work.  The
test name needs to be stable, diagnostics identifying why or where it
failed should be separate prints.

Actually, prompted by the comments below about test variants I've now
run the test and see that what's in -next is also broken in that it's
running a lot of the tests twice with sealing enabled or disabled but
not including this in the reported test name resulting in most of the
tests reporting like this:

   ok 11 test_seal_mprotect
   ok 12 test_seal_mprotect

which is also going to confuse automated systems, they have a hard time
working out which instance is which (generally the test numbers get
ignored between runs as they're not at all stable).  The test names need
to roll in the parameterisation:

   ok 11 test_seal_mprotect seal=true
   ok 12 test_seal_mprotect seal=false

(or something, the specific format doesn't matter so long as the names
are both stable and distinct).

> Having line number does help dev to go to error directly, and I'm not
> against filling in the "action" field, but you might also agree with
> me, finding unique text for each error would require some decent
> amount of time, especially for large tests such as mseal_test.

In these situations if it's a typical Unix API function setting errno
that failed I tend to end up writing diagnostics like:
	
	ksft_perror("open()")

possibly with some of the arguments included as well, or something
equivalently basic for other kinds of error.  This is fairly mindless so
quick and easy to do and more robust against line number slips if you're
not looking at exactly the same version of the code, sometimes it's even
enough you don't even need to look at the test to understand why it's
upset.

> > Honestly this just sounds and looks like kselftest_harness.h, it's
> > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
> > for asserts.  The main gotchas with it are that it's not particularly

> OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture.

> If I  switch to test_fixture, e,g, using TEST(test_name)

> how do I pass the "seal" flag to it ?
> e.g. how do I run the same test twice, first seal = true, and second seal=false.

>         test_seal_mmap_shrink(false);
>         test_seal_mmap_shrink(true);

That looks like fixture variants to me, using those with
kselftest_harness.h will also fix the problem with duplicate test names
being used since it generates different names for each instance of the
test.  Something like:

FIXTURE(with_seal)
{
};

FIXTURE_VARIANT(with_seal)
{
	bool seal;
};

FIXTURE_VARIANT_ADD(with_seal, yes)
{
	.seal = true,
};

FIXTURE_VARIANT_ADD(with_seal, no)
{
	.seal = false,
};

FIXTURE_SETUP(with_seal)
{
}

FIXTURE_TEARDOWN(with_seal)
{
}

then a bunch of tests using that fixture:

TEST_F(with_seal, test_seal_mmap_shrink)
{
	if (variant->seal) {
		/* setup sealing */
	}

	...
}

TEST_F(with_seal, test_seal_mprotect)
{
	if (variant->seal) {
		/* setup sealing */
	}

	...
}

You don't need to actually set up anything in your fixture, but you do
need to have setup and teardown functions so the framework can emit
required boilerplate.  The gcs-locking.c test I recently added in -next 
is an example of a similar thing where we just need the variants,
there's no actual fixture.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu]

HI Mark

On Fri, Oct 18, 2024 at 2:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote:
> > On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote:
> > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote:
>
> > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split:
> > > line:2573" which automated systems aren't going to be able to associate
> > > with the passing "test_munmap_free_multiple_ranges_with_split", nor with
> > > any failures that occur on any other lines in the function.
>
> > I see. That will happen when those tests are modified and line number
> > changes. I could see reasoning for this argument, especially when
> > those tests are flaky and get updated often.
>
> > In practice, I hope any of those kernel self-test failures should get
> > fixed immediately, or even better, run before dev submitting the patch
> > that affects the mm area.
>
> That's not the entire issue - it is also a problem that the test name
> is not the same between passes and failures so automated systems can't
> associate the failures with the passes.
I failed to understand this part.
Maybe you meant the failing logging  is not the same across the
multiple versions of test code, by testname you meant "failing
logging"

>When a test starts failing they
> will see the passing test disappear and a new test appears that has never
> worked.
 > This will mean that for example if they have bisection support
> or UI for showing when a test started regressing those won't work.  The
> test name needs to be stable, diagnostics identifying why or where it
> failed should be separate prints.
>
If the test hasn't been changed for a while,  and start failing. Then
it is quite easy to run the same test on recent code changes. I think
you might agree with me on this. The only thing that bisec needs to
check is if the entire tests are failing or not.

I haven't used the biset functionality, so I'm not sure how it works
exactly, e.g. when it runs on the old version of kernel, does it use
the test binary from the old kernel ? or the test binary provided by
dev ?

> Actually, prompted by the comments below about test variants I've now
> run the test and see that what's in -next is also broken in that it's
> running a lot of the tests twice with sealing enabled or disabled but
> not including this in the reported test name resulting in most of the
> tests reporting like this:
>
>    ok 11 test_seal_mprotect
>    ok 12 test_seal_mprotect
>
> which is also going to confuse automated systems, they have a hard time
> working out which instance is which (generally the test numbers get
> ignored between runs as they're not at all stable).  The test names need
> to roll in the parameterisation:
>
>    ok 11 test_seal_mprotect seal=true
>    ok 12 test_seal_mprotect seal=false
>
> (or something, the specific format doesn't matter so long as the names
> are both stable and distinct).
>
Yes. Agree that this is a limitation of this macro.

> > Having line number does help dev to go to error directly, and I'm not
> > against filling in the "action" field, but you might also agree with
> > me, finding unique text for each error would require some decent
> > amount of time, especially for large tests such as mseal_test.
>
> In these situations if it's a typical Unix API function setting errno
> that failed I tend to end up writing diagnostics like:
>
>         ksft_perror("open()")
>
> possibly with some of the arguments included as well, or something
> equivalently basic for other kinds of error.  This is fairly mindless so
> quick and easy to do and more robust against line number slips if you're
> not looking at exactly the same version of the code, sometimes it's even
> enough you don't even need to look at the test to understand why it's
> upset.
>
I understand what you are saying, but personally, I still think line
numbers are a faster and more direct way to failure.

> > > Honestly this just sounds and looks like kselftest_harness.h, it's
> > > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for
> > > for asserts.  The main gotchas with it are that it's not particularly
>
> > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture.
>
> > If I  switch to test_fixture, e,g, using TEST(test_name)
>
> > how do I pass the "seal" flag to it ?
> > e.g. how do I run the same test twice, first seal = true, and second seal=false.
>
> >         test_seal_mmap_shrink(false);
> >         test_seal_mmap_shrink(true);
>
> That looks like fixture variants to me, using those with
> kselftest_harness.h will also fix the problem with duplicate test names
> being used since it generates different names for each instance of the
> test.  Something like:
>
> FIXTURE(with_seal)
> {
> };
>
> FIXTURE_VARIANT(with_seal)
> {
>         bool seal;
> };
>
> FIXTURE_VARIANT_ADD(with_seal, yes)
> {
>         .seal = true,
> };
>
> FIXTURE_VARIANT_ADD(with_seal, no)
> {
>         .seal = false,
> };
>
> FIXTURE_SETUP(with_seal)
> {
> }
>
> FIXTURE_TEARDOWN(with_seal)
> {
> }
>
> then a bunch of tests using that fixture:
>
> TEST_F(with_seal, test_seal_mmap_shrink)
> {
>         if (variant->seal) {
>                 /* setup sealing */
>         }
>
>         ...
> }
>
> TEST_F(with_seal, test_seal_mprotect)
> {
>         if (variant->seal) {
>                 /* setup sealing */
>         }
>
>         ...
> }
>
> You don't need to actually set up anything in your fixture, but you do
> need to have setup and teardown functions so the framework can emit
> required boilerplate.  The gcs-locking.c test I recently added in -next
> is an example of a similar thing where we just need the variants,
> there's no actual fixture.

Thanks! This is really helpful, I think the existing mseal_test can be
quickly converted using this example.

(A side note: if selftest documentation is updated to include this
example, it will be much easier to future dev to follow)

Thanks
-Jeff

Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]

On Fri, Oct 18, 2024 at 05:10:01PM -0700, Jeff Xu wrote:
> On Fri, Oct 18, 2024 at 2:05 PM Mark Brown <broonie@kernel.org> wrote:

> > That's not the entire issue - it is also a problem that the test name
> > is not the same between passes and failures so automated systems can't
> > associate the failures with the passes.

> I failed to understand this part.
> Maybe you meant the failing logging  is not the same across the
> multiple versions of test code, by testname you meant "failing
> logging"

Tests are identified by the string given in the line reporting their
result, that's not *really* a log message but rather a test name.  The
strings for a given test need to be the same between different runs of
the test program for tooling to be able to see that it's the same test.

> >When a test starts failing they
> > will see the passing test disappear and a new test appears that has never
> > worked.
>  > This will mean that for example if they have bisection support
> > or UI for showing when a test started regressing those won't work.  The
> > test name needs to be stable, diagnostics identifying why or where it
> > failed should be separate prints.

> If the test hasn't been changed for a while,  and start failing. Then

Well, you'd hope that the tests never start failing but yet we still
have tests and keep running them.  

> it is quite easy to run the same test on recent code changes. I think
> you might agree with me on this. The only thing that bisec needs to
> check is if the entire tests are failing or not.

Unfortunately we're not in a position where people can reliably assume
that every test program will always work everywhere so people work on
individual tests, and it's certainly useful for UIs to be able to give
an overview of what specifically failed.  A bunch of that is tests that
just don't implement feature detection/skipping properly.

> I haven't used the biset functionality, so I'm not sure how it works
> exactly, e.g. when it runs on the old version of kernel, does it use
> the test binary from the old kernel ? or the test binary provided by
> dev ?

That's up to whoever is doing the testing, but I think most people run
the selftests from the version of the code they're testing.  Some of the
subsystems aren't very enthusiastic about supporting running on older
kernels.

> > > how do I pass the "seal" flag to it ?
> > > e.g. how do I run the same test twice, first seal = true, and second seal=false.
> >
> > >         test_seal_mmap_shrink(false);
> > >         test_seal_mmap_shrink(true);

> > That looks like fixture variants to me, using those with
> > kselftest_harness.h will also fix the problem with duplicate test names
> > being used since it generates different names for each instance of the
> > test.  Something like:

> Thanks! This is really helpful, I think the existing mseal_test can be
> quickly converted using this example.

Great!

> (A side note: if selftest documentation is updated to include this
> example, it will be much easier to future dev to follow)

Possibly send a patch adding that wherever you were looking?  That was
just a quick hack down of the gcs-locking program verifying that it had
what I thought you needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 5/5] selftests/mseal: add more tests for mremap

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Add sealing test to cover mremap for
Expand/shrink/move across vmas.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 tools/testing/selftests/mm/mseal_test.c | 202 +++++++++++++++++++++++-
 1 file changed, 201 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index 3516389034a7..fee655bbbf0a 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -2339,6 +2339,197 @@ static void test_seal_mmap_reuse_addr(bool seal)
 	REPORT_TEST_PASS();
 }
 
+static void test_seal_mremap_shrink_multiple_vmas(bool seal)
+{
+	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);
+
+	ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	if (seal) {
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	ret2 = sys_mremap(ptr, 12 * page_size, 6 * page_size, 0, 0);
+	if (seal) {
+		FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
+		FAIL_TEST_IF_FALSE(errno == EPERM);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+		size = get_vma_size(ptr + 4 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+	} else {
+		FAIL_TEST_IF_FALSE(ret2 == ptr);
+
+		size = get_vma_size(ptr, &prot);
+		FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+		size = get_vma_size(ptr + 4 * page_size, &prot);
+		FAIL_TEST_IF_FALSE(size == 2 * page_size);
+	}
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mremap_expand_multiple_vmas(bool seal)
+{
+	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);
+
+	ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	/* ummap last 4 pages. */
+	ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	if (seal) {
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	ret2 = sys_mremap(ptr, 8 * page_size, 12 * page_size, 0, 0);
+	FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mremap_move_expand_multiple_vmas(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 12 * page_size;
+	int ret;
+	void *ret2;
+	int prot;
+	void *ptr2;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	setup_single_address(size, &ptr2);
+	FAIL_TEST_IF_FALSE(ptr2 != (void *)-1);
+
+	ret = sys_munmap(ptr2, 12 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	/* ummap last 4 pages. */
+	ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	if (seal) {
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	/* move and expand cross VMA boundary will fail */
+	ret2 = sys_mremap(ptr, 8 * page_size, 10 * page_size, MREMAP_FIXED | MREMAP_MAYMOVE, ptr2);
+	FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+
+	REPORT_TEST_PASS();
+}
+
+static void test_seal_mremap_move_shrink_multiple_vmas(bool seal)
+{
+	void *ptr;
+	unsigned long page_size = getpagesize();
+	unsigned long size = 12 * page_size;
+	int ret;
+	void *ret2;
+	int prot;
+	void *ptr2;
+
+	setup_single_address(size, &ptr);
+	FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+	setup_single_address(size, &ptr2);
+	FAIL_TEST_IF_FALSE(ptr2 != (void *)-1);
+
+	ret = sys_munmap(ptr2, 12 * page_size);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE);
+	FAIL_TEST_IF_FALSE(!ret);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 4);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 0);
+
+	if (seal) {
+		ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+		FAIL_TEST_IF_FALSE(!ret);
+	}
+
+	/* move and shrink cross VMA boundary is NOK */
+	ret2 = sys_mremap(ptr, 12 * page_size, 8 * page_size, MREMAP_FIXED | MREMAP_MAYMOVE, ptr2);
+	FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED);
+
+	size = get_vma_size(ptr, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 4);
+
+	size = get_vma_size(ptr + 4 * page_size, &prot);
+	FAIL_TEST_IF_FALSE(size == 4 * page_size);
+	FAIL_TEST_IF_FALSE(prot == 0);
+
+	REPORT_TEST_PASS();
+}
+
 int main(int argc, char **argv)
 {
 	bool test_seal = seal_support();
@@ -2360,7 +2551,7 @@ int main(int argc, char **argv)
 	if (!get_vma_size_supported())
 		ksft_exit_skip("get_vma_size not supported\n");
 
-	ksft_set_plan(97);
+	ksft_set_plan(105);
 
 	test_seal_addseal();
 	test_seal_unmapped_start();
@@ -2481,5 +2672,14 @@ int main(int argc, char **argv)
 	test_seal_mmap_reuse_addr(false);
 	test_seal_mmap_reuse_addr(true);
 
+	test_seal_mremap_shrink_multiple_vmas(false);
+	test_seal_mremap_shrink_multiple_vmas(true);
+	test_seal_mremap_expand_multiple_vmas(false);
+	test_seal_mremap_expand_multiple_vmas(true);
+	test_seal_mremap_move_expand_multiple_vmas(false);
+	test_seal_mremap_move_expand_multiple_vmas(true);
+	test_seal_mremap_move_shrink_multiple_vmas(false);
+	test_seal_mremap_move_shrink_multiple_vmas(true);
+
 	ksft_finished();
 }
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH v3 0/5] Increase mseal test coverage

[Lorenzo Stoakes]

On Fri, Aug 30, 2024 at 06:02:32PM GMT, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> This series increase the test coverage of mseal_test by:
>
> Add check for vma_size, prot, and error code for existing tests.
> Add more testcases for madvise, munmap, mmap and mremap to cover
> sealing in different scenarios.
>
> The increase test coverage hopefully help to prevent future regression.
> It doesn't change any existing mm api's semantics, i.e. it will pass on
> linux main and 6.10 branch.

This is a big improvement in detail, thanks.

>
> Note: in order to pass this test in mm-unstable, mm-unstable must have
> Liam's fix on mmap [1]
>
> [1] https://lore.kernel.org/linux-kselftest/vyllxuh5xbqmaoyl2mselebij5ox7cseekjcvl5gmzoxxwd2he@hxi4mpjanxzt/#t

This is already in mm-unstable so this is redundant, and as Liam explained,
his new v8 series will contain this fix too, and his old version will be
unwound before new applied, so at no time will this be relevant.

>
> History:
> V3:
> - no-functional change, incooperate feedback from Pedro Falcato
>
> V2:
> - https://lore.kernel.org/linux-kselftest/20240829214352.963001-1-jeffxu@chromium.org/
> - remove the mmap fix (Liam R. Howlett will fix it separately)
> - Add cover letter (Lorenzo Stoakes)

I think I asked for more than this :)

> - split the testcase for ease of review (Mark Brown)
>
> V1:
> - https://lore.kernel.org/linux-kselftest/20240828225522.684774-1-jeffxu@chromium.org/
>
>
> Jeff Xu (5):
>   selftests/mseal_test: Check vma_size, prot, error code.
>   selftests/mseal: add sealed madvise type
>   selftests/mseal: munmap across multiple vma ranges.
>   selftests/mseal: add more tests for mmap
>   selftests/mseal: add more tests for mremap
>
>  tools/testing/selftests/mm/mseal_test.c | 830 ++++++++++++++++++++++--
>  1 file changed, 763 insertions(+), 67 deletions(-)
>
> --
> 2.46.0.469.g59c65b2a67-goog
>

Overall having checked one patch in the series, I am quite concerned that
these tests are not testing what they suggest they do, are redundant, and I
have found numerous problems line-by-line.

I've also encountered copy/pasted blocks, comparing PROT_xxx fields to
magic numbers, and it generally feels really rushed.

I feel like it might be worth taking some time on the next respin to really
think carefully about how these functions work, checking man pages and
source, and getting some understanding of what it is we really need to
assert about mseal() behaviour.

We're here to help you and want to be collaborative and help get mseal()
into a good state, so I'd like to suggest taking your time on the next
respin to really improve the quality and think carefully in each instance
_what_ you are testing and _why_.

And don't be afraid to ask questions and discuss these things with
us. We're happy to help!

Anyway, I am now off on a long weekend before my birthday, I wish you a
great weekend and hope we can find a way to move forward constructively
with this! :)

Thanks, Lorenzo