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