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

[David Hildenbrand <david@redhat.com>]

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