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