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