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

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

On 4/14/22 2:19 PM, Shuah Khan wrote:
> 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.
Sure
> The description is a bit confusing. What happens when mmap tries
> to map addresses < mmap_min_addr?
>
If run without root, mmap returns the EPERM error code but with root

it returns the EACCES error code. In the code of the test case, it checks

if the map failed with the EPERM error code and will retry with a new 
address.

The test breaks when run as root because EACCES isn't caught for the 
retry condition.

Rather than arbitrarily also catching the EACCES error code, I avoid mapping

with 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?
>
In the original code, addr starts at 0, attempts an mmap, and retries on

failure with addr += c.src_alignment. I just retry earlier before the mmap

call because the mmap call will always fail if the 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