[v2] selftest/vm: add mremap expand merge offset test

Commit Message

Lorenzo Stoakes Jan. 2, 2023, 2:44 p.m. UTC

Add a test to assert that we can mremap() and expand a mapping starting
from an offset within an existing mapping. We unmap the last page in a 3
page mapping to ensure that the remap should always succeed, before
remapping from the 2nd page.

This is additionally a regression test for the issue solved in "mm, mremap:
fix mremap() expanding vma with addr inside vma" and confirmed to fail
prior to the change and pass after it.

Finally, this patch updates the existing mremap expand merge test to check
error conditions and reduce code duplication between the two tests.

Signed-off-by: Lorenzo Stoakes <[e-mail]>
---
 tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
 1 file changed, 93 insertions(+), 22 deletions(-)

Comments

David Hildenbrand Jan. 2, 2023, 3:34 p.m. UTC | #1

On 02.01.23 15:44, Lorenzo Stoakes wrote:
> Add a test to assert that we can mremap() and expand a mapping starting
> from an offset within an existing mapping. We unmap the last page in a 3
> page mapping to ensure that the remap should always succeed, before
> remapping from the 2nd page.
> 
> This is additionally a regression test for the issue solved in "mm, mremap:
> fix mremap() expanding vma with addr inside vma" and confirmed to fail
> prior to the change and pass after it.
> 
> Finally, this patch updates the existing mremap expand merge test to check
> error conditions and reduce code duplication between the two tests.
> 
> Signed-off-by: Lorenzo Stoakes <[e-mail]>
> ---
>   tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
>   1 file changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c


...

> +
> +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +
> +	if (start == MAP_FAILED) {
> +		ksft_print_msg("mmap failed: %s\n", strerror(errno));

I'd

	ksft_test_result_fail(...)
	return;

> +		goto out;
> +	}
> +
> +	munmap(start + page_size, page_size);
> +	remap = mremap(start, page_size, 2 * page_size, 0);
> +	if (remap == MAP_FAILED) {
> +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> +		munmap(start, page_size);
> +		munmap(start + 2 * page_size, page_size);
> +		goto out;

dito

	ksft_test_result_fail(...)
	...
	return;

> +	}
> +
> +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> +	munmap(start, 3 * page_size);
> +
> +out:

then you can drop the out label.

> +	if (success)
> +		ksft_test_result_pass("%s\n", test_name);
> +	else
> +		ksft_test_result_fail("%s\n", test_name);
> +}
> +
> +/*
> + * Similar to mremap_expand_merge() except instead of removing the middle page,
> + * we remove the last then attempt to remap offset from the second page. This
> + * should result in the mapping being restored to its former state.
> + */
> +static void mremap_expand_merge_offset(FILE *maps_fp, unsigned long page_size)
> +{
> +
> +	char *test_name = "mremap expand merge offset";
> +	bool success = false;
> +	char *remap, *start;
> +
> +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +
> +	if (start == MAP_FAILED) {
> +		ksft_print_msg("mmap failed: %s\n", strerror(errno));
> +		goto out;
> +	}
> +
> +	/* Unmap final page to ensure we have space to expand. */
> +	munmap(start + 2 * page_size, page_size);
> +	remap = mremap(start + page_size, page_size, 2 * page_size, 0);
> +	if (remap == MAP_FAILED) {
> +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> +		munmap(start, 2 * page_size);
> +		goto out;
> +	}
> +
> +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> +	munmap(start, 3 * page_size);
> +
> +out:

dito.

>   	if (success)
>   		ksft_test_result_pass("%s\n", test_name);
>   	else
>   		ksft_test_result_fail("%s\n", test_name);
> -	fclose(fp);
>   }
>   
>   /*
> @@ -385,6 +447,7 @@ int main(int argc, char **argv)
>   	struct test perf_test_cases[MAX_PERF_TEST];
>   	int page_size;
>   	time_t t;
> +	FILE *maps_fp;

I'd simply use a global variable, same applies for page_size. But 
passing it around is also ok.

>   
>   	pattern_seed = (unsigned int) time(&t);
>   
> @@ -458,7 +521,15 @@ int main(int argc, char **argv)
>   		run_mremap_test_case(test_cases[i], &failures, threshold_mb,
>   				     pattern_seed);
>   
> -	mremap_expand_merge(page_size);
> +	maps_fp = fopen("/proc/self/maps", "r");
> +	if (maps_fp == NULL) {
> +		ksft_print_msg("Failed to read /proc/self/maps: %s\n", strerror(errno));

Maybe simply fail the test completely and return -errno ?

> +	} else {
> +		mremap_expand_merge(maps_fp, page_size);
> +		mremap_expand_merge_offset(maps_fp, page_size);
> +
> +		fclose(maps_fp);

No need to fclose, just keep it open ...

> +	}
>   
>   	if (run_perf_tests) {
>   		ksft_print_msg("\n%s\n",


Acked-by: David Hildenbrand <[e-mail]>

Lorenzo Stoakes Jan. 2, 2023, 3:49 p.m. UTC | #2

On Mon, Jan 02, 2023 at 04:34:14PM +0100, David Hildenbrand wrote:
> On 02.01.23 15:44, Lorenzo Stoakes wrote:
> > Add a test to assert that we can mremap() and expand a mapping starting
> > from an offset within an existing mapping. We unmap the last page in a 3
> > page mapping to ensure that the remap should always succeed, before
> > remapping from the 2nd page.
> >
> > This is additionally a regression test for the issue solved in "mm, mremap:
> > fix mremap() expanding vma with addr inside vma" and confirmed to fail
> > prior to the change and pass after it.
> >
> > Finally, this patch updates the existing mremap expand merge test to check
> > error conditions and reduce code duplication between the two tests.
> >
> > Signed-off-by: Lorenzo Stoakes <[e-mail]>
> > ---
> >   tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
> >   1 file changed, 93 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
>
>
> ...
>
> > +
> > +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +
> > +	if (start == MAP_FAILED) {
> > +		ksft_print_msg("mmap failed: %s\n", strerror(errno));
>
> I'd
>
> 	ksft_test_result_fail(...)
> 	return;
>
> > +		goto out;
> > +	}
> > +
> > +	munmap(start + page_size, page_size);
> > +	remap = mremap(start, page_size, 2 * page_size, 0);
> > +	if (remap == MAP_FAILED) {
> > +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > +		munmap(start, page_size);
> > +		munmap(start + 2 * page_size, page_size);
> > +		goto out;
>
> dito
>
> 	ksft_test_result_fail(...)
> 	...
> 	return;
>
> > +	}
> > +
> > +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> > +	munmap(start, 3 * page_size);
> > +
> > +out:
>
> then you can drop the out label.
>

I have to disagree on this, to be consistent with the other tests the
failure messages should include the test name, and putting the
ksft_test_result_fail("test name") in each branch as well as the error
message would just be wilful duplication.

I do think it's a pity C lacks mechanisms such that gotos are sometimes
necessary, but I can only right so many wrongs in this patch :)

> > +	if (success)
> > +		ksft_test_result_pass("%s\n", test_name);
> > +	else
> > +		ksft_test_result_fail("%s\n", test_name);
> > +}
> > +
> > +/*
> > + * Similar to mremap_expand_merge() except instead of removing the middle page,
> > + * we remove the last then attempt to remap offset from the second page. This
> > + * should result in the mapping being restored to its former state.
> > + */
> > +static void mremap_expand_merge_offset(FILE *maps_fp, unsigned long page_size)
> > +{
> > +
> > +	char *test_name = "mremap expand merge offset";
> > +	bool success = false;
> > +	char *remap, *start;
> > +
> > +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +
> > +	if (start == MAP_FAILED) {
> > +		ksft_print_msg("mmap failed: %s\n", strerror(errno));
> > +		goto out;
> > +	}
> > +
> > +	/* Unmap final page to ensure we have space to expand. */
> > +	munmap(start + 2 * page_size, page_size);
> > +	remap = mremap(start + page_size, page_size, 2 * page_size, 0);
> > +	if (remap == MAP_FAILED) {
> > +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > +		munmap(start, 2 * page_size);
> > +		goto out;
> > +	}
> > +
> > +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
> > +	munmap(start, 3 * page_size);
> > +
> > +out:
>
> dito.
>
> >   	if (success)
> >   		ksft_test_result_pass("%s\n", test_name);
> >   	else
> >   		ksft_test_result_fail("%s\n", test_name);
> > -	fclose(fp);
> >   }
> >   /*
> > @@ -385,6 +447,7 @@ int main(int argc, char **argv)
> >   	struct test perf_test_cases[MAX_PERF_TEST];
> >   	int page_size;
> >   	time_t t;
> > +	FILE *maps_fp;
>
> I'd simply use a global variable, same applies for page_size. But passing it
> around is also ok.
>

I am trying to keep things consistent with what's gone before in this code,
and given page_size is being passed around I think the 'when in Rome'
principle applies equally to passing the fp around I think.

> >   	pattern_seed = (unsigned int) time(&t);
> > @@ -458,7 +521,15 @@ int main(int argc, char **argv)
> >   		run_mremap_test_case(test_cases[i], &failures, threshold_mb,
> >   				     pattern_seed);
> > -	mremap_expand_merge(page_size);
> > +	maps_fp = fopen("/proc/self/maps", "r");
> > +	if (maps_fp == NULL) {
> > +		ksft_print_msg("Failed to read /proc/self/maps: %s\n", strerror(errno));
>
> Maybe simply fail the test completely and return -errno ?
>

Ack on this one, will spin a v3 with this as otherwise we might miss test
failures here.

> > +	} else {
> > +		mremap_expand_merge(maps_fp, page_size);
> > +		mremap_expand_merge_offset(maps_fp, page_size);
> > +
> > +		fclose(maps_fp);
>
> No need to fclose, just keep it open ...
>

I'd rather we did fclose() to keep things clean, as who knows what
additional tests may be added later and to be pedantic about matching an
fopen() with an fclose().

> > +	}
> >   	if (run_perf_tests) {
> >   		ksft_print_msg("\n%s\n",
>
>
> Acked-by: David Hildenbrand <[e-mail]>
>

Thanks!

> --
> Thanks,
>
> David / dhildenb
>

David Hildenbrand Jan. 2, 2023, 3:53 p.m. UTC | #3

On 02.01.23 16:49, Lorenzo Stoakes wrote:
> On Mon, Jan 02, 2023 at 04:34:14PM +0100, David Hildenbrand wrote:
>> On 02.01.23 15:44, Lorenzo Stoakes wrote:
>>> Add a test to assert that we can mremap() and expand a mapping starting
>>> from an offset within an existing mapping. We unmap the last page in a 3
>>> page mapping to ensure that the remap should always succeed, before
>>> remapping from the 2nd page.
>>>
>>> This is additionally a regression test for the issue solved in "mm, mremap:
>>> fix mremap() expanding vma with addr inside vma" and confirmed to fail
>>> prior to the change and pass after it.
>>>
>>> Finally, this patch updates the existing mremap expand merge test to check
>>> error conditions and reduce code duplication between the two tests.
>>>
>>> Signed-off-by: Lorenzo Stoakes <[e-mail]>
>>> ---
>>>    tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++-----
>>>    1 file changed, 93 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
>>
>>
>> ...
>>
>>> +
>>> +	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
>>> +		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>> +
>>> +	if (start == MAP_FAILED) {
>>> +		ksft_print_msg("mmap failed: %s\n", strerror(errno));
>>
>> I'd
>>
>> 	ksft_test_result_fail(...)
>> 	return;
>>
>>> +		goto out;
>>> +	}
>>> +
>>> +	munmap(start + page_size, page_size);
>>> +	remap = mremap(start, page_size, 2 * page_size, 0);
>>> +	if (remap == MAP_FAILED) {
>>> +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
>>> +		munmap(start, page_size);
>>> +		munmap(start + 2 * page_size, page_size);
>>> +		goto out;
>>
>> dito
>>
>> 	ksft_test_result_fail(...)
>> 	...
>> 	return;
>>
>>> +	}
>>> +
>>> +	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
>>> +	munmap(start, 3 * page_size);
>>> +
>>> +out:
>>
>> then you can drop the out label.
>>
> 
> I have to disagree on this, to be consistent with the other tests the
> failure messages should include the test name, and putting the
> ksft_test_result_fail("test name") in each branch as well as the error
> message would just be wilful duplication.
> 
> I do think it's a pity C lacks mechanisms such that gotos are sometimes
> necessary, but I can only right so many wrongs in this patch :)
> 

Let's agree to disagree ;) Too bad we don't have prefix push/pop 
functionality as we have in other similar testing frameworks -- to avoid 
exactly that duplication.

[...]

>> I'd simply use a global variable, same applies for page_size. But passing it
>> around is also ok.
>>
> 
> I am trying to keep things consistent with what's gone before in this code,
> and given page_size is being passed around I think the 'when in Rome'
> principle applies equally to passing the fp around I think.

Other tests we have handle it "easier". :)

Patch

diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
index 9496346973d4..8946283b0ba5 100644
--- a/tools/testing/selftests/vm/mremap_test.c
+++ b/tools/testing/selftests/vm/mremap_test.c
@@ -119,47 +119,109 @@  static unsigned long long get_mmap_min_addr(void)
 }
 
 /*
- * This test validates that merge is called when expanding a mapping.
- * Mapping containing three pages is created, middle page is unmapped
- * and then the mapping containing the first page is expanded so that
- * it fills the created hole. The two parts should merge creating
- * single mapping with three pages.
+ * Using /proc/self/maps, assert that the specified address range is contained
+ * within a single mapping.
  */
-static void mremap_expand_merge(unsigned long page_size)
+static bool is_range_mapped(FILE *maps_fp, void *start, void *end)
 {
-	char *test_name = "mremap expand merge";
-	FILE *fp;
 	char *line = NULL;
 	size_t len = 0;
 	bool success = false;
-	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
-			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-
-	munmap(start + page_size, page_size);
-	mremap(start, page_size, 2 * page_size, 0);
 
-	fp = fopen("/proc/self/maps", "r");
-	if (fp == NULL) {
-		ksft_test_result_fail("%s\n", test_name);
-		return;
-	}
+	rewind(maps_fp);
 
-	while (getline(&line, &len, fp) != -1) {
+	while (getline(&line, &len, maps_fp) != -1) {
 		char *first = strtok(line, "- ");
 		void *first_val = (void *)strtol(first, NULL, 16);
 		char *second = strtok(NULL, "- ");
 		void *second_val = (void *) strtol(second, NULL, 16);
 
-		if (first_val == start && second_val == start + 3 * page_size) {
+		if (first_val <= start && second_val >= end) {
 			success = true;
 			break;
 		}
 	}
+
+	return success;
+}
+
+/*
+ * This test validates that merge is called when expanding a mapping.
+ * Mapping containing three pages is created, middle page is unmapped
+ * and then the mapping containing the first page is expanded so that
+ * it fills the created hole. The two parts should merge creating
+ * single mapping with three pages.
+ */
+static void mremap_expand_merge(FILE *maps_fp, unsigned long page_size)
+{
+	char *test_name = "mremap expand merge";
+	bool success = false;
+	char *remap, *start;
+
+	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
+		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	if (start == MAP_FAILED) {
+		ksft_print_msg("mmap failed: %s\n", strerror(errno));
+		goto out;
+	}
+
+	munmap(start + page_size, page_size);
+	remap = mremap(start, page_size, 2 * page_size, 0);
+	if (remap == MAP_FAILED) {
+		ksft_print_msg("mremap failed: %s\n", strerror(errno));
+		munmap(start, page_size);
+		munmap(start + 2 * page_size, page_size);
+		goto out;
+	}
+
+	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
+	munmap(start, 3 * page_size);
+
+out:
+	if (success)
+		ksft_test_result_pass("%s\n", test_name);
+	else
+		ksft_test_result_fail("%s\n", test_name);
+}
+
+/*
+ * Similar to mremap_expand_merge() except instead of removing the middle page,
+ * we remove the last then attempt to remap offset from the second page. This
+ * should result in the mapping being restored to its former state.
+ */
+static void mremap_expand_merge_offset(FILE *maps_fp, unsigned long page_size)
+{
+
+	char *test_name = "mremap expand merge offset";
+	bool success = false;
+	char *remap, *start;
+
+	start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
+		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	if (start == MAP_FAILED) {
+		ksft_print_msg("mmap failed: %s\n", strerror(errno));
+		goto out;
+	}
+
+	/* Unmap final page to ensure we have space to expand. */
+	munmap(start + 2 * page_size, page_size);
+	remap = mremap(start + page_size, page_size, 2 * page_size, 0);
+	if (remap == MAP_FAILED) {
+		ksft_print_msg("mremap failed: %s\n", strerror(errno));
+		munmap(start, 2 * page_size);
+		goto out;
+	}
+
+	success = is_range_mapped(maps_fp, start, start + 3 * page_size);
+	munmap(start, 3 * page_size);
+
+out:
 	if (success)
 		ksft_test_result_pass("%s\n", test_name);
 	else
 		ksft_test_result_fail("%s\n", test_name);
-	fclose(fp);
 }
 
 /*
@@ -385,6 +447,7 @@  int main(int argc, char **argv)
 	struct test perf_test_cases[MAX_PERF_TEST];
 	int page_size;
 	time_t t;
+	FILE *maps_fp;
 
 	pattern_seed = (unsigned int) time(&t);
 
@@ -458,7 +521,15 @@  int main(int argc, char **argv)
 		run_mremap_test_case(test_cases[i], &failures, threshold_mb,
 				     pattern_seed);
 
-	mremap_expand_merge(page_size);
+	maps_fp = fopen("/proc/self/maps", "r");
+	if (maps_fp == NULL) {
+		ksft_print_msg("Failed to read /proc/self/maps: %s\n", strerror(errno));
+	} else {
+		mremap_expand_merge(maps_fp, page_size);
+		mremap_expand_merge_offset(maps_fp, page_size);
+
+		fclose(maps_fp);
+	}
 
 	if (run_perf_tests) {
 		ksft_print_msg("\n%s\n",