Message-ID: <20230828201716.GF1621761@google.com>
Date:   Mon, 28 Aug 2023 20:17:16 +0000
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Lorenzo Stoakes <lstoakes@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, Shuah Khan <shuah@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michal Hocko <mhocko@...e.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kirill A Shutemov <kirill@...temov.name>,
        "Liam R. Howlett" <liam.howlett@...cle.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Lokesh Gidra <lokeshgidra@...gle.com>
Subject: Re: [PATCH v5 7/7] selftests: mm: Add a test for moving from an
 offset from start of mapping

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@...lfernandes.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@...lfernandes.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@...il.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

Powered by blists - more mailing lists