On Tue, Aug 22, 2023 at 01:55:00AM +0000, Joel Fernandes (Google) wrote:
> From: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxx>
> ---
> 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.
>
> 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
you changed anything or not (I eyeballed and seems like you didn't though!)
> /*
> * This test validates that merge is called when expanding a mapping.
> * Mapping containing three pages is created, middle page is unmapped
> @@ -300,60 +355,6 @@ static void mremap_move_within_range(char pattern_seed)
> ksft_test_result_fail("%s\n", test_name);
> }
>
> -/*
> - * 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;
> -}
> -
> /* Returns the time taken for the remap on success else returns -1. */
> static long long remap_region(struct config c, unsigned int threshold_mb,
> char pattern_seed)
> @@ -487,6 +488,83 @@ static long long remap_region(struct config c, unsigned int threshold_mb,
> return ret;
> }
>
> +/*
> + * 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?
> + 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...
> +
> + 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?
Unrelated to this patch, but I guess it might be tricky to come up with a
test for the shift_arg_pages() stack case?
Irrespective of the above, this looks correct to me so,
Reviewed-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
Thanks for putting in so much effort on the testing side too! :)
