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

Message ID 420e491062db9151504aef5661c8a2d928ef6bd7.1672675224.git.lstoakes@gmail.com
State New
Headers
Series [v3] selftest/vm: add mremap expand merge offset test |

Commit Message

Lorenzo Stoakes Jan. 2, 2023, 4:01 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 <lstoakes@gmail.com>
---
 tools/testing/selftests/vm/mremap_test.c | 116 ++++++++++++++++++-----
 1 file changed, 94 insertions(+), 22 deletions(-)
  

Comments

David Hildenbrand Jan. 2, 2023, 4:03 p.m. UTC | #1
> -	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));
> +		exit(KSFT_FAIL);
> +	} else {

With the exit() in place, the else branch is implicit and the else can 
be dropped.

> +		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",

Thanks!

Acked-by: David Hildenbrand <david@redhat.com>
  
Lorenzo Stoakes Jan. 2, 2023, 4:09 p.m. UTC | #2
On Mon, Jan 02, 2023 at 05:03:31PM +0100, David Hildenbrand wrote:

> With the exit() in place, the else branch is implicit and the else can be
> dropped.

Good point, we'll go to a v4 then :) that does neaten things up
actually. Hopefully this should suffice!

Agree with your previous point about tooling by the way, it would be good to
have some more functionality in place that wraps these places where you feel the
need to goto.
  
David Hildenbrand Jan. 2, 2023, 4:10 p.m. UTC | #3
On 02.01.23 17:09, Lorenzo Stoakes wrote:
> On Mon, Jan 02, 2023 at 05:03:31PM +0100, David Hildenbrand wrote:
> 
>> With the exit() in place, the else branch is implicit and the else can be
>> dropped.
> 
> Good point, we'll go to a v4 then :) that does neaten things up
> actually. Hopefully this should suffice!

Maybe wait until tomorrow for feedback from others. Make sure to include 
my acked-by.

Thanks :)
  
Lorenzo Stoakes Jan. 2, 2023, 4:16 p.m. UTC | #4
On Mon, Jan 02, 2023 at 05:10:40PM +0100, David Hildenbrand wrote:
> Maybe wait until tomorrow for feedback from others. Make sure to include my
> acked-by.

Too late... I thought Andrew typically added these kind of things himself? As an
Acked-by could be a reply to an unmodified patch for example. Though I guess
it'd save you having to reply to the v4? Anyway sorry, already sent it!

It's sat in the mailing list so others can review further if required, this is a
bank holiday in the UK, and as a part-time contributor (and otherwise distracted
by book work) it is a rare moment to be able to respond to feedback so will tend
to get squashed into evenings/weekends/public holidays :) If people have further
comments I will, of course, respond as + when I can!

Cheers!
  
David Hildenbrand Jan. 2, 2023, 4:20 p.m. UTC | #5
On 02.01.23 17:16, Lorenzo Stoakes wrote:
> On Mon, Jan 02, 2023 at 05:10:40PM +0100, David Hildenbrand wrote:
>> Maybe wait until tomorrow for feedback from others. Make sure to include my
>> acked-by.
> 
> Too late... I thought Andrew typically added these kind of things himself? As an
> Acked-by could be a reply to an unmodified patch for example. Though I guess
> it'd save you having to reply to the v4? Anyway sorry, already sent it!

Maintainers usually apply tags that are sent to the latest version, when 
picking up the patch. Maintainers usually refrain from going through 
multiple earlier patch versions to pick up tags -- especially, because 
some changes might require a submitter from dropping tags (e.g., bigger 
changes) and the tags might no longer be valid.

So better always include tags when resending, so they don't get lost.

> 
> It's sat in the mailing list so others can review further if required, this is a
> bank holiday in the UK, and as a part-time contributor (and otherwise distracted
> by book work) it is a rare moment to be able to respond to feedback so will tend
> to get squashed into evenings/weekends/public holidays :) If people have further
> comments I will, of course, respond as + when I can!
> 
> Cheers!

Happy holidays then :)
  
Lorenzo Stoakes Jan. 2, 2023, 4:22 p.m. UTC | #6
On Mon, Jan 02, 2023 at 05:20:49PM +0100, David Hildenbrand wrote:
> Maintainers usually apply tags that are sent to the latest version, when
> picking up the patch. Maintainers usually refrain from going through
> multiple earlier patch versions to pick up tags -- especially, because some
> changes might require a submitter from dropping tags (e.g., bigger changes)
> and the tags might no longer be valid.
>
> So better always include tags when resending, so they don't get lost.
>

Ack, apologies again, and that's completely reasonable. Would you mind adding to
the v4 thread just so we have it? Going forward I'll make sure to propagate the
tags!

Thanks for the review!
  

Patch

diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
index 9496346973d4..b8e427b2538c 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,16 @@  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));
+		exit(KSFT_FAIL);
+	} 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",