On Sun, Aug 27, 2023 at 11:12:14AM +0100, Lorenzo Stoakes wrote:
On Tue, Aug 22, 2023 at 01:55:00AM +0000, Joel Fernandes (Google) wrote:
From: Joel Fernandes joel@joelfernandes.org
It is possible that the aligned address falls on no existing mapping, however that does not mean that we can just align it down to that. This test verifies that the "vma->vm_start != addr_to_align" check in can_align_down() prevents disastrous results if aligning down when source and dest are mutually aligned within a PMD but the source/dest addresses requested are not at the beginning of the respective mapping containing these addresses.
Signed-off-by: Joel Fernandes (Google) joel@joelfernandes.org
tools/testing/selftests/mm/mremap_test.c | 189 ++++++++++++++++------- 1 file changed, 134 insertions(+), 55 deletions(-)
diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c index f45d1abedc9c..c71ac8306c89 100644 --- a/tools/testing/selftests/mm/mremap_test.c +++ b/tools/testing/selftests/mm/mremap_test.c @@ -24,6 +24,7 @@
#define MIN(X, Y) ((X) < (Y) ? (X) : (Y)) #define SIZE_MB(m) ((size_t)m * (1024 * 1024)) +#define SIZE_KB(k) ((size_t)k * 1024)
Same comment as previous re: wrapping k in parens, it doesn't really matter here that much but for good practice (and consistency with MIN()) would be nice to do.
Ack.
struct config { unsigned long long src_alignment; @@ -148,6 +149,60 @@ static bool is_range_mapped(FILE *maps_fp, void *start, void *end) return success; }
+/*
- Returns the start address of the mapping on success, else returns
- NULL on failure.
- */
+static void *get_source_mapping(struct config c) +{
- unsigned long long addr = 0ULL;
- void *src_addr = NULL;
- unsigned long long mmap_min_addr;
- mmap_min_addr = get_mmap_min_addr();
- /*
* For some tests, we need to not have any mappings below the* source mapping. Add some headroom to mmap_min_addr for this.*/- mmap_min_addr += 10 * _4MB;
+retry:
- addr += c.src_alignment;
- if (addr < mmap_min_addr)
goto retry;- src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE,
MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,-1, 0);- if (src_addr == MAP_FAILED) {
if (errno == EPERM || errno == EEXIST)goto retry;goto error;- }
- /*
* Check that the address is aligned to the specified alignment.* Addresses which have alignments that are multiples of that* specified are not considered valid. For instance, 1GB address is* 2MB-aligned, however it will not be considered valid for a* requested alignment of 2MB. This is done to reduce coincidental* alignment in the tests.*/- if (((unsigned long long) src_addr & (c.src_alignment - 1)) ||
!((unsigned long long) src_addr & c.src_alignment)) {munmap(src_addr, c.region_size);goto retry;- }
- if (!src_addr)
goto error;- return src_addr;
+error:
- ksft_print_msg("Failed to map source region: %s\n",
strerror(errno));- return NULL;
+}
A meta thing, but it'd be nice to separate out _moving_ functions into their own patch to make reviewing easier as here it's not obvious whether
Sure, good point and I'll give that a shot.
[..]
+/*
- Verify that an mremap aligning down does not destroy
- the beginning of the mapping just because the aligned
- down address landed on a mapping that maybe does not exist.
- */
+static void mremap_move_1mb_from_start(char pattern_seed) +{
- char *test_name = "mremap move 1mb from start at 2MB+256KB aligned src";
- void *src = NULL, *dest = NULL;
- int i, success = 1;
- /* Config to reuse get_source_mapping() to do an aligned mmap. */
- struct config c = {
.src_alignment = SIZE_MB(1) + SIZE_KB(256),.region_size = SIZE_MB(6)- };
- src = get_source_mapping(c);
- if (!src) {
success = 0;goto out;- }
- c.src_alignment = SIZE_MB(1) + SIZE_KB(256);
Why are you setting this again?
Yes, I may not need to but since 'c' is passed to a function, I just do it for robustness. If you'd like, I can drop it.
- dest = get_source_mapping(c);
- if (!dest) {
success = 0;goto out;- }
- /* Set byte pattern for source block. */
- srand(pattern_seed);
- for (i = 0; i < SIZE_MB(2); i++) {
((char *)src)[i] = (char) rand();- }
- /*
* Unmap the beginning of dest so that the aligned address* falls on no mapping.*/- munmap(dest, SIZE_MB(1));
This actually aligns (no pun intended) with my comments on the min mmap address + 4 MiB comments previously. But I guess for the generalised mremap test you will still need to have that headroom...
Yes, I guess the situation you are validly concerned about is if someone mapped something just when I unmapped. However, as this is test code it probably should be OK?
- void *new_ptr = mremap(src + SIZE_MB(1), SIZE_MB(1), SIZE_MB(1),
MREMAP_MAYMOVE | MREMAP_FIXED, dest + SIZE_MB(1));- if (new_ptr == MAP_FAILED) {
perror("mremap");success = 0;goto out;- }
- /* Verify byte pattern after remapping */
- srand(pattern_seed);
- for (i = 0; i < SIZE_MB(1); i++) {
char c = (char) rand();if (((char *)src)[i] != c) {ksft_print_msg("Data at src at %d got corrupted due to unrelated mremap\n",i);ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff,((char *) src)[i] & 0xff);success = 0;}- }
+out:
- if (src && munmap(src, c.region_size) == -1)
perror("munmap src");- if (dest && munmap(dest, c.region_size) == -1)
perror("munmap dest");- if (success)
ksft_test_result_pass("%s\n", test_name);- else
ksft_test_result_fail("%s\n", test_name);+}
static void run_mremap_test_case(struct test test_case, int *failures, unsigned int threshold_mb, unsigned int pattern_seed) @@ -565,7 +643,7 @@ int main(int argc, char **argv) unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD; unsigned int pattern_seed; int num_expand_tests = 2;
- int num_misc_tests = 1;
- int num_misc_tests = 2; struct test test_cases[MAX_TEST] = {}; struct test perf_test_cases[MAX_PERF_TEST]; int page_size;
@@ -666,6 +744,7 @@ int main(int argc, char **argv) fclose(maps_fp);
mremap_move_within_range(pattern_seed);
mremap_move_1mb_from_start(pattern_seed);
if (run_perf_tests) { ksft_print_msg("\n%s\n",
-- 2.42.0.rc1.204.g551eb34607-goog
Have you verified this test fails if you eliminate the vma->vm_start != addr_to_align check?
Yes. When Linus found the issue, I wrote this very test to ensure that we caught it and that the fix made the test pass.
Unrelated to this patch, but I guess it might be tricky to come up with a test for the shift_arg_pages() stack case?
Yes I think so, at the moment I am resorting to manually hacking the kernel to test that which is not ideal but..
Irrespective of the above, this looks correct to me so,
Reviewed-by: Lorenzo Stoakes lstoakes@gmail.com Thanks for putting in so much effort on the testing side too! :)
My pleasure, and thanks for the awesome review! I'll get to work on it and post a new version soon.
thanks,
- Joel