Re: [PATCH 1/4] selftest/vm: verify mmap addr in mremap_test

[Shuah Khan <skhan@linuxfoundation.org>]

On 4/14/22 11:15 AM, Sidhartha Kumar wrote:
> Avoid calling mmap with requested addresses that
> are less than the system's mmap_min_addr. Running
> the test as root returns EACCES when trying to map
> addresses < mmap_min_addr which is not one of the
> error codes for the retry condition. Add a munmap
> call after an alignment check as the mappings are
> retained after the retry and can reach vm.max_map_count.
> 

Please use 75 or 76 chars per line in change logs to make it
easier to read the commit log.

The description is a bit confusing. What happens when mmap tries
to map addresses < mmap_min_addr?

> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>   tools/testing/selftests/vm/mremap_test.c | 41 +++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> index 0624d1bd71b5..58600fee4b81 100644
> --- a/tools/testing/selftests/vm/mremap_test.c
> +++ b/tools/testing/selftests/vm/mremap_test.c
> @@ -6,6 +6,7 @@
>   
>   #include <errno.h>
>   #include <stdlib.h>
> +#include <stdio.h>
>   #include <string.h>
>   #include <sys/mman.h>
>   #include <time.h>
> @@ -64,6 +65,35 @@ enum {
>   	.expect_failure = should_fail				\
>   }
>   
> +/* Returns mmap_min_addr sysctl */

Change this to "sysctl tunable from procfs"

> +static unsigned long long get_mmap_min_addr(void)
> +{
> +	FILE *fp;
> +	int n_matched;
> +	static unsigned long long addr;
> +
> +	if (addr)
> +		return addr;
> +
> +	fp = fopen("/proc/sys/vm/mmap_min_addr", "r");
> +	if (fp == NULL) {
> +		ksft_print_msg("Failed to open /proc/sys/vm/mmap_min_addr: %s\n",
> +			strerror(errno));
> +		exit(KSFT_SKIP);
> +	}
> +
> +	n_matched = fscanf(fp, "%llu", &addr);
> +	if (n_matched != 1) {
> +		ksft_print_msg("Failed to read /proc/sys/vm/mmap_min_addr: %s\n",
> +			strerror(errno));
> +		fclose(fp);
> +		exit(KSFT_SKIP);
> +	}
> +
> +	fclose(fp);
> +	return addr;
> +}
> +
>   /*
>    * Returns the start address of the mapping on success, else returns
>    * NULL on failure.
> @@ -72,8 +102,15 @@ static void *get_source_mapping(struct config c)
>   {
>   	unsigned long long addr = 0ULL;
>   	void *src_addr = NULL;
> +	unsigned long long mmap_min_addr;
> +
> +	mmap_min_addr = get_mmap_min_addr();
> +
>   retry:
>   	addr += c.src_alignment;
> +	if (addr < mmap_min_addr)
> +		goto retry;

Should this be an error or retry? Can you add why this is a retry
when addr < mmap_min_addr?

> +
>   	src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE,
>   			MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
>   			-1, 0);
> @@ -91,8 +128,10 @@ static void *get_source_mapping(struct config c)
>   	 * alignment in the tests.
>   	 */
>   	if (((unsigned long long) src_addr & (c.src_alignment - 1)) ||
> -			!((unsigned long long) src_addr & c.src_alignment))
> +			!((unsigned long long) src_addr & c.src_alignment)) {
> +		munmap(src_addr, c.region_size);
>   		goto retry;
> +	}
>   
>   	if (!src_addr)
>   		goto error;
> 

thanks,
-- Shuah