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

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

[Lorenzo Stoakes]

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 <lstoakes@gmail.com>
---
 tools/testing/selftests/vm/mremap_test.c | 111 +++++++++++++++++++----
 1 file changed, 93 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
index 9496346973d4..28a17d4e8afd 100644
--- a/tools/testing/selftests/vm/mremap_test.c
+++ b/tools/testing/selftests/vm/mremap_test.c
@@ -119,30 +119,19 @@ 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(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;
-	}
+	if (fp == NULL)
+		return false;

 	while (getline(&line, &len, fp) != -1) {
 		char *first = strtok(line, "- ");
@@ -150,16 +139,101 @@ static void mremap_expand_merge(unsigned long page_size)
 		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;
 		}
 	}
+
+	fclose(fp);
+	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(unsigned long page_size)
+{
+	char *test_name = "mremap expand merge";
+	bool success = false;
+	int errsv = 0;
+	char *remap;
+	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
+			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	if (start == MAP_FAILED) {
+		errsv = errno;
+		goto error;
+	}
+
+	munmap(start + page_size, page_size);
+	remap = mremap(start, page_size, 2 * page_size, 0);
+	if (remap == MAP_FAILED) {
+		errsv = errno;
+		munmap(start, page_size);
+		munmap(start + 2 * page_size, page_size);
+		goto error;
+	}
+
+	success = is_range_mapped(start, start + 3 * page_size);
+
+	munmap(start, 3 * page_size);
+	goto out;
+
+error:
+	ksft_print_msg("Unexpected mapping/remapping error: %s\n",
+		       strerror(errsv));
+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(unsigned long page_size)
+{
+
+	char *test_name = "mremap expand merge offset";
+	bool success = false;
+	int errsv = 0;
+	char *remap;
+	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
+			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	if (start == MAP_FAILED) {
+		errsv = errno;
+		goto error;
+	}
+
+	/* 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) {
+		errsv = errno;
+		munmap(start, 2 * page_size);
+		goto error;
+	}
+
+	success = is_range_mapped(start, start + 3 * page_size);
+	goto out;
+
+error:
+	ksft_print_msg("Unexpected mapping/remapping error: %s\n",
+		       strerror(errsv));
+out:
 	if (success)
 		ksft_test_result_pass("%s\n", test_name);
 	else
 		ksft_test_result_fail("%s\n", test_name);
-	fclose(fp);
 }

 /*
@@ -459,6 +533,7 @@ int main(int argc, char **argv)
 				     pattern_seed);

 	mremap_expand_merge(page_size);
+	mremap_expand_merge_offset(page_size);

 	if (run_perf_tests) {
 		ksft_print_msg("\n%s\n",
--
2.38.1

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

[David Hildenbrand]

On 16.12.22 22: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 <lstoakes@gmail.com>
> ---
>   tools/testing/selftests/vm/mremap_test.c | 111 +++++++++++++++++++----
>   1 file changed, 93 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> index 9496346973d4..28a17d4e8afd 100644
> --- a/tools/testing/selftests/vm/mremap_test.c
> +++ b/tools/testing/selftests/vm/mremap_test.c
> @@ -119,30 +119,19 @@ 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(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;
> -	}
> +	if (fp == NULL)
> +		return false;

This is unexpected. It would be valuable to ksft_print_msg("[INFO] .." 
something, indicating that we don't know because we cannot access that info.

ksft_print_msg("[INFO] Opening /proc/self/maps failed"


But I'd even suggest opening "/proc/self/maps" once in main() and 
failing directly there. Then we don't have to worry about it here.

> 
>   	while (getline(&line, &len, fp) != -1) {
>   		char *first = strtok(line, "- ");
> @@ -150,16 +139,101 @@ static void mremap_expand_merge(unsigned long page_size)
>   		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;
>   		}
>   	}
> +
> +	fclose(fp);
> +	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(unsigned long page_size)
> +{
> +	char *test_name = "mremap expand merge";
> +	bool success = false;
> +	int errsv = 0;
> +	char *remap;
> +	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> +			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

I'd suggest


	char *remap, *start;

	start = mmap()
	if (start == MAP_FAILED) { ...

to make this easier to read.

> +
> +	if (start == MAP_FAILED) {
> +		errsv = errno;
> +		goto error;
> +	}
> +
> +	munmap(start + page_size, page_size);
> +	remap = mremap(start, page_size, 2 * page_size, 0);
> +	if (remap == MAP_FAILED) {
> +		errsv = errno;
> +		munmap(start, page_size);
> +		munmap(start + 2 * page_size, page_size);
> +		goto error;
> +	}
> +
> +	success = is_range_mapped(start, start + 3 * page_size);
> +
> +	munmap(start, 3 * page_size);
> +	goto out;
> +
> +error:
> +	ksft_print_msg("Unexpected mapping/remapping error: %s\n",
> +		       strerror(errsv));

Please avoid the "error" label and just print proper errors directly at 
the two callsites. Then, remove the "goto out".

> +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(unsigned long page_size)
> +{
> +
> +	char *test_name = "mremap expand merge offset";
> +	bool success = false;
> +	int errsv = 0;
> +	char *remap;
> +	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> +			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

Dito.

> +
> +	if (start == MAP_FAILED) {
> +		errsv = errno;
> +		goto error;
> +	}
> +
> +	/* 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) {
> +		errsv = errno;
> +		munmap(start, 2 * page_size);
> +		goto error;
> +	}
> +
> +	success = is_range_mapped(start, start + 3 * page_size);
> +	goto out;
> +
> +error:
> +	ksft_print_msg("Unexpected mapping/remapping error: %s\n",
> +		       strerror(errsv));

Dito.

> +out:


-- 
Thanks,

David / dhildenb

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

[Lorenzo Stoakes]

On Mon, Jan 02, 2023 at 02:32:55PM +0100, David Hildenbrand wrote:
> On 16.12.22 22: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 <lstoakes@gmail.com>
> > ---
> >   tools/testing/selftests/vm/mremap_test.c | 111 +++++++++++++++++++----
> >   1 file changed, 93 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> > index 9496346973d4..28a17d4e8afd 100644
> > --- a/tools/testing/selftests/vm/mremap_test.c
> > +++ b/tools/testing/selftests/vm/mremap_test.c
> > @@ -119,30 +119,19 @@ 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(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;
> > -	}
> > +	if (fp == NULL)
> > +		return false;
>
> This is unexpected. It would be valuable to ksft_print_msg("[INFO] .."
> something, indicating that we don't know because we cannot access that info.
>
> ksft_print_msg("[INFO] Opening /proc/self/maps failed"
>
>
> But I'd even suggest opening "/proc/self/maps" once in main() and failing
> directly there. Then we don't have to worry about it here.
>
> >
> >   	while (getline(&line, &len, fp) != -1) {
> >   		char *first = strtok(line, "- ");
> > @@ -150,16 +139,101 @@ static void mremap_expand_merge(unsigned long page_size)
> >   		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;
> >   		}
> >   	}
> > +
> > +	fclose(fp);
> > +	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(unsigned long page_size)
> > +{
> > +	char *test_name = "mremap expand merge";
> > +	bool success = false;
> > +	int errsv = 0;
> > +	char *remap;
> > +	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > +			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>
> I'd suggest
>
>
> 	char *remap, *start;
>
> 	start = mmap()
> 	if (start == MAP_FAILED) { ...
>
> to make this easier to read.
>
> > +
> > +	if (start == MAP_FAILED) {
> > +		errsv = errno;
> > +		goto error;
> > +	}
> > +
> > +	munmap(start + page_size, page_size);
> > +	remap = mremap(start, page_size, 2 * page_size, 0);
> > +	if (remap == MAP_FAILED) {
> > +		errsv = errno;
> > +		munmap(start, page_size);
> > +		munmap(start + 2 * page_size, page_size);
> > +		goto error;
> > +	}
> > +
> > +	success = is_range_mapped(start, start + 3 * page_size);
> > +
> > +	munmap(start, 3 * page_size);
> > +	goto out;
> > +
> > +error:
> > +	ksft_print_msg("Unexpected mapping/remapping error: %s\n",
> > +		       strerror(errsv));
>
> Please avoid the "error" label and just print proper errors directly at the
> two callsites. Then, remove the "goto out".
>
> > +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(unsigned long page_size)
> > +{
> > +
> > +	char *test_name = "mremap expand merge offset";
> > +	bool success = false;
> > +	int errsv = 0;
> > +	char *remap;
> > +	char *start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE,
> > +			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>
> Dito.
>
> > +
> > +	if (start == MAP_FAILED) {
> > +		errsv = errno;
> > +		goto error;
> > +	}
> > +
> > +	/* 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) {
> > +		errsv = errno;
> > +		munmap(start, 2 * page_size);
> > +		goto error;
> > +	}
> > +
> > +	success = is_range_mapped(start, start + 3 * page_size);
> > +	goto out;
> > +
> > +error:
> > +	ksft_print_msg("Unexpected mapping/remapping error: %s\n",
> > +		       strerror(errsv));
>
> Dito.
>
> > +out:
>
>
> --
> Thanks,
>
> David / dhildenb
>

Ack on all, will spin a v2. Thanks for review!