[PATCH 0/4] selftest/vm fix segfault in mremap_test

[PATCH 0/4] selftest/vm fix segfault in mremap_test

[Sidhartha Kumar]

The mremap test currently segfaults because mremap
does not have a NOREPLACE flag which will fail if the
remap destination address collides with an existing mapping.
The segfault is caused by the mremap call destorying the
text region mapping of the program. This patch series fixes
the segfault by sanitizing the mremap destination address
and introduces other minor fixes to the test case. 

Sidhartha Kumar (4):
  selftest/vm: verify mmap addr in mremap_test
  selftest/vm: verify remap destination address in mremap_test
  selftest/vm: support xfail in mremap_test
  selftest/vm: add skip support to mremap_test

 tools/testing/selftests/vm/mremap_test.c  | 79 ++++++++++++++++++++++-
 tools/testing/selftests/vm/run_vmtests.sh | 11 +++-
 2 files changed, 85 insertions(+), 5 deletions(-)

-- 
2.27.0

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

[Sidhartha Kumar]

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.

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 */
+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;
+
 	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;
-- 
2.27.0

[PATCH 2/4] selftest/vm: verify remap destination address in mremap_test

[Sidhartha Kumar]

Because mremap does not have a NOREPLACE flag,
it can destroy existing mappings. This can
cause a segfault if regions such as text are
destroyed. Verify the requested mremap destination
address does not overlap any existing mappings
by using mmap's FIXED_NOREPLACE flag and checking
for the EEXIST error code. Keep incrementing the
destination address until a valid mapping is found
or max address is reached.

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 tools/testing/selftests/vm/mremap_test.c | 36 ++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
index 58600fee4b81..98e9cff34aa7 100644
--- a/tools/testing/selftests/vm/mremap_test.c
+++ b/tools/testing/selftests/vm/mremap_test.c
@@ -10,6 +10,7 @@
 #include <string.h>
 #include <sys/mman.h>
 #include <time.h>
+#include <limits.h>
 
 #include "../kselftest.h"
 
@@ -65,6 +66,34 @@ enum {
 	.expect_failure = should_fail				\
 }
 
+/*
+ * Returns 0 if the requested remap region overlaps with an
+ * existing mapping (e.g text, stack) else returns 1.
+ */
+static int remap_region_valid(void *addr, unsigned long long size)
+{
+	void *remap_addr = NULL;
+	int ret = 1;
+
+	if ((unsigned long long) addr > ULLONG_MAX - size) {
+		ksft_print_msg("Can't find a valid region to remap to\n");
+		exit(KSFT_SKIP);
+	}
+
+	/* Use MAP_FIXED_NOREPLACE flag to ensure region is not mapped */
+	remap_addr = mmap(addr, size, PROT_READ | PROT_WRITE,
+			MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
+			-1, 0);
+	if (remap_addr == MAP_FAILED) {
+		if (errno == EEXIST)
+			ret = 0;
+	} else {
+		munmap(remap_addr, size);
+	}
+
+	return ret;
+}
+
 /* Returns mmap_min_addr sysctl */
 static unsigned long long get_mmap_min_addr(void)
 {
@@ -180,6 +209,13 @@ static long long remap_region(struct config c, unsigned int threshold_mb,
 	if (!((unsigned long long) addr & c.dest_alignment))
 		addr = (void *) ((unsigned long long) addr | c.dest_alignment);
 
+	/* Don't destroy existing mappings unless expected to overlap */
+	while (!remap_region_valid(addr, c.region_size)) {
+		if (c.overlapping)
+			break;
+		addr += c.src_alignment;
+	}
+
 	clock_gettime(CLOCK_MONOTONIC, &t_start);
 	dest_addr = mremap(src_addr, c.region_size, c.region_size,
 			MREMAP_MAYMOVE|MREMAP_FIXED, (char *) addr);
-- 
2.27.0

[PATCH 3/4] selftest/vm: support xfail in mremap_test

[Sidhartha Kumar]

Use ksft_test_result_xfail for the tests which
are expected to fail.

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 tools/testing/selftests/vm/mremap_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
index 98e9cff34aa7..ace9c3596ed7 100644
--- a/tools/testing/selftests/vm/mremap_test.c
+++ b/tools/testing/selftests/vm/mremap_test.c
@@ -269,7 +269,7 @@ static void run_mremap_test_case(struct test test_case, int *failures,
 
 	if (remap_time < 0) {
 		if (test_case.expect_failure)
-			ksft_test_result_pass("%s\n\tExpected mremap failure\n",
+			ksft_test_result_xfail("%s\n\tExpected mremap failure\n",
 					      test_case.name);
 		else {
 			ksft_test_result_fail("%s\n", test_case.name);
-- 
2.27.0

[PATCH 4/4] selftest/vm: add skip support to mremap_test

[Sidhartha Kumar]

Allow the mremap test to be skipped due to errors
such as failing to find a valid remap region and
failure to parse the mmap_min_addr sysctl.

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 tools/testing/selftests/vm/run_vmtests.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
index 88e15fbb5027..eae98f5de2cc 100755
--- a/tools/testing/selftests/vm/run_vmtests.sh
+++ b/tools/testing/selftests/vm/run_vmtests.sh
@@ -272,11 +272,16 @@ echo "-------------------"
 echo "running mremap_test"
 echo "-------------------"
 ./mremap_test
-if [ $? -ne 0 ]; then
+ret_val=$?
+
+if [ $ret_val -eq 0 ]; then
+	echo "[PASS]"
+elif [ $ret_val -eq $ksft_skip ]; then
+	 echo "[SKIP]"
+	 exitcode=$ksft_skip
+else
 	echo "[FAIL]"
 	exitcode=1
-else
-	echo "[PASS]"
 fi
 
 echo "-----------------"
-- 
2.27.0

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

[Shuah Khan]

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

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

[Sidhartha Kumar]

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

Re: [PATCH 2/4] selftest/vm: verify remap destination address in mremap_test

[Shuah Khan]

On 4/14/22 11:15 AM, Sidhartha Kumar wrote:
> Because mremap does not have a NOREPLACE flag,
> it can destroy existing mappings. This can
> cause a segfault if regions such as text are
> destroyed.

Please explain the reason for segfault.

Add a blank line here. Makes it easier to read.

Verify the requested mremap destination
> address does not overlap any existing mappings
> by using mmap's FIXED_NOREPLACE flag and checking

Spell this out fully - MAP_FIXED_NOREPLACE
> for the EEXIST error code. Keep incrementing the
> destination address until a valid mapping is found
> or max address is reached.
> 

Essentially mremap() doesn't check for overlaps and removes
or overwrites existing mappings? The way you are fixing it
is by verifying by calling mremap() with MAP_FIXED_NOREPLACE
flag and check for EEXIST.

What happens when max address is reached?

Same comment on # of chars per line in commit log. Also

> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>   tools/testing/selftests/vm/mremap_test.c | 36 ++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> index 58600fee4b81..98e9cff34aa7 100644
> --- a/tools/testing/selftests/vm/mremap_test.c
> +++ b/tools/testing/selftests/vm/mremap_test.c
> @@ -10,6 +10,7 @@
>   #include <string.h>
>   #include <sys/mman.h>
>   #include <time.h>
> +#include <limits.h>
>   
>   #include "../kselftest.h"
>   
> @@ -65,6 +66,34 @@ enum {
>   	.expect_failure = should_fail				\
>   }
>   
> +/*
> + * Returns 0 if the requested remap region overlaps with an
> + * existing mapping (e.g text, stack) else returns 1.
> + */
> +static int remap_region_valid(void *addr, unsigned long long size)

This returns bool 0 (false) 1 (true)

Please name the routine - is_remap_region_valid() and change it to
return bool.

> +{
> +	void *remap_addr = NULL;
> +	int ret = 1;
> +
> +	if ((unsigned long long) addr > ULLONG_MAX - size) {
> +		ksft_print_msg("Can't find a valid region to remap to\n");

Change it to "Couldn't" - also this message doesn't look right. We hav't
looked for valid region yet and it just exceeds the limits?


> +		exit(KSFT_SKIP);> +	}
> +
> +	/* Use MAP_FIXED_NOREPLACE flag to ensure region is not mapped */
> +	remap_addr = mmap(addr, size, PROT_READ | PROT_WRITE,
> +			MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
> +			-1, 0);

Alignment should match open parenthesis here and in other places. Makes it
easier to read the code.

> +	if (remap_addr == MAP_FAILED) {
> +		if (errno == EEXIST)
> +			ret = 0;
> +	} else {
> +		munmap(remap_addr, size);
> +	}
> +
> +	return ret;
> +}
> +
>   /* Returns mmap_min_addr sysctl */
>   static unsigned long long get_mmap_min_addr(void)
>   {
> @@ -180,6 +209,13 @@ static long long remap_region(struct config c, unsigned int threshold_mb,
>   	if (!((unsigned long long) addr & c.dest_alignment))
>   		addr = (void *) ((unsigned long long) addr | c.dest_alignment);
>   
> +	/* Don't destroy existing mappings unless expected to overlap */
> +	while (!remap_region_valid(addr, c.region_size)) {
> +		if (c.overlapping)
> +			break;
> +		addr += c.src_alignment;
> +	}
> +
>   	clock_gettime(CLOCK_MONOTONIC, &t_start);
>   	dest_addr = mremap(src_addr, c.region_size, c.region_size,
>   			MREMAP_MAYMOVE|MREMAP_FIXED, (char *) addr);
> 

thanks,
-- Shuah

Re: [PATCH 3/4] selftest/vm: support xfail in mremap_test

[Shuah Khan]

On 4/14/22 11:15 AM, Sidhartha Kumar wrote:
> Use ksft_test_result_xfail for the tests which
> are expected to fail.
> 

Same Nit about commit log lines.

> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>   tools/testing/selftests/vm/mremap_test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
> index 98e9cff34aa7..ace9c3596ed7 100644
> --- a/tools/testing/selftests/vm/mremap_test.c
> +++ b/tools/testing/selftests/vm/mremap_test.c
> @@ -269,7 +269,7 @@ static void run_mremap_test_case(struct test test_case, int *failures,
>   
>   	if (remap_time < 0) {
>   		if (test_case.expect_failure)
> -			ksft_test_result_pass("%s\n\tExpected mremap failure\n",
> +			ksft_test_result_xfail("%s\n\tExpected mremap failure\n",
>   					      test_case.name);
>   		else {
>   			ksft_test_result_fail("%s\n", test_case.name);
> 

Thank you. Looks good to me.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH 4/4] selftest/vm: add skip support to mremap_test

[Shuah Khan]

On 4/14/22 11:15 AM, Sidhartha Kumar wrote:
> Allow the mremap test to be skipped due to errors
> such as failing to find a valid remap region and
> failure to parse the mmap_min_addr sysctl.
> 
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>   tools/testing/selftests/vm/run_vmtests.sh | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
> index 88e15fbb5027..eae98f5de2cc 100755
> --- a/tools/testing/selftests/vm/run_vmtests.sh
> +++ b/tools/testing/selftests/vm/run_vmtests.sh
> @@ -272,11 +272,16 @@ echo "-------------------"
>   echo "running mremap_test"
>   echo "-------------------"
>   ./mremap_test
> -if [ $? -ne 0 ]; then
> +ret_val=$?
> +
> +if [ $ret_val -eq 0 ]; then
> +	echo "[PASS]"
> +elif [ $ret_val -eq $ksft_skip ]; then
> +	 echo "[SKIP]"
> +	 exitcode=$ksft_skip
> +else
>   	echo "[FAIL]"
>   	exitcode=1
> -else
> -	echo "[PASS]"
>   fi
>   
>   echo "-----------------"
> 

Thank you. Looks good to me.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH 2/4] selftest/vm: verify remap destination address in mremap_test

[Sidhartha Kumar]

On 4/14/22 2:47 PM, Shuah Khan wrote:
> On 4/14/22 11:15 AM, Sidhartha Kumar wrote:
>> Because mremap does not have a NOREPLACE flag,
>> it can destroy existing mappings. This can
>> cause a segfault if regions such as text are
>> destroyed.
>
> Please explain the reason for segfault.
>
With the MREMAP_FIXED flag used by the test,
the text region, which fell in the range of the remap
region, got unmapped. This caused a segfault when
trying to fetch the next instruction after the mremap()
call.
> Add a blank line here. Makes it easier to read.
>
> Verify the requested mremap destination
>> address does not overlap any existing mappings
>> by using mmap's FIXED_NOREPLACE flag and checking
>
> Spell this out fully - MAP_FIXED_NOREPLACE
>> for the EEXIST error code. Keep incrementing the
>> destination address until a valid mapping is found
>> or max address is reached.
>>
>
> Essentially mremap() doesn't check for overlaps and removes
> or overwrites existing mappings? The way you are fixing it
> is by verifying by calling mremap() with MAP_FIXED_NOREPLACE
> flag and check for EEXIST.
>
Yes, with the MREMAP_FIXED flag that the test uses, any previous
mapping in the address range of the remap region gets unmapped.
Yes, fixing this issue by calling mmap() with MAP_FIXED_NOREPLACE
flag and checking for EEXIST.

> What happens when max address is reached?
>
That is covered by the check if (addr > ULLONG_MAX - region size)
in the remap_region_valid() function.
> Same comment on # of chars per line in commit log. Also
>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>>   tools/testing/selftests/vm/mremap_test.c | 36 ++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/tools/testing/selftests/vm/mremap_test.c 
>> b/tools/testing/selftests/vm/mremap_test.c
>> index 58600fee4b81..98e9cff34aa7 100644
>> --- a/tools/testing/selftests/vm/mremap_test.c
>> +++ b/tools/testing/selftests/vm/mremap_test.c
>> @@ -10,6 +10,7 @@
>>   #include <string.h>
>>   #include <sys/mman.h>
>>   #include <time.h>
>> +#include <limits.h>
>>     #include "../kselftest.h"
>>   @@ -65,6 +66,34 @@ enum {
>>       .expect_failure = should_fail                \
>>   }
>>   +/*
>> + * Returns 0 if the requested remap region overlaps with an
>> + * existing mapping (e.g text, stack) else returns 1.
>> + */
>> +static int remap_region_valid(void *addr, unsigned long long size)
>
> This returns bool 0 (false) 1 (true)
>
> Please name the routine - is_remap_region_valid() and change it to
> return bool.
>
>> +{
>> +    void *remap_addr = NULL;
>> +    int ret = 1;
>> +
>> +    if ((unsigned long long) addr > ULLONG_MAX - size) {
>> +        ksft_print_msg("Can't find a valid region to remap to\n");
>
> Change it to "Couldn't" - also this message doesn't look right. We hav't
> looked for valid region yet and it just exceeds the limits?
>
Because this function is called in a loop in remap_region() and addr is 
being
incremented continuously, we could enter this function with addr high 
enough that
another increment would cause overflow.
>
>> +        exit(KSFT_SKIP);> +    }
>> +
>> +    /* Use MAP_FIXED_NOREPLACE flag to ensure region is not mapped */
>> +    remap_addr = mmap(addr, size, PROT_READ | PROT_WRITE,
>> +            MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
>> +            -1, 0);
>
> Alignment should match open parenthesis here and in other places. 
> Makes it
> easier to read the code.
>
>> +    if (remap_addr == MAP_FAILED) {
>> +        if (errno == EEXIST)
>> +            ret = 0;
>> +    } else {
>> +        munmap(remap_addr, size);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   /* Returns mmap_min_addr sysctl */
>>   static unsigned long long get_mmap_min_addr(void)
>>   {
>> @@ -180,6 +209,13 @@ static long long remap_region(struct config c, 
>> unsigned int threshold_mb,
>>       if (!((unsigned long long) addr & c.dest_alignment))
>>           addr = (void *) ((unsigned long long) addr | 
>> c.dest_alignment);
>>   +    /* Don't destroy existing mappings unless expected to overlap */
>> +    while (!remap_region_valid(addr, c.region_size)) {
>> +        if (c.overlapping)
>> +            break;
>> +        addr += c.src_alignment;
>> +    }
>> +
>>       clock_gettime(CLOCK_MONOTONIC, &t_start);
>>       dest_addr = mremap(src_addr, c.region_size, c.region_size,
>>               MREMAP_MAYMOVE|MREMAP_FIXED, (char *) addr);
>>
>
> thanks,
> -- Shuah
>

Re: [PATCH 2/4] selftest/vm: verify remap destination address in mremap_test

[Shuah Khan]

On 4/14/22 4:24 PM, Sidhartha Kumar wrote:
> 
> 
> On 4/14/22 2:47 PM, Shuah Khan wrote:
>> On 4/14/22 11:15 AM, Sidhartha Kumar wrote:
>>> Because mremap does not have a NOREPLACE flag,
>>> it can destroy existing mappings. This can
>>> cause a segfault if regions such as text are
>>> destroyed.
>>
>> Please explain the reason for segfault.
>>
> With the MREMAP_FIXED flag used by the test,
> the text region, which fell in the range of the remap
> region, got unmapped. This caused a segfault when
> trying to fetch the next instruction after the mremap()
> call.
>> Add a blank line here. Makes it easier to read.
>>
>> Verify the requested mremap destination
>>> address does not overlap any existing mappings
>>> by using mmap's FIXED_NOREPLACE flag and checking
>>
>> Spell this out fully - MAP_FIXED_NOREPLACE
>>> for the EEXIST error code. Keep incrementing the
>>> destination address until a valid mapping is found
>>> or max address is reached.
>>>
>>
>> Essentially mremap() doesn't check for overlaps and removes
>> or overwrites existing mappings? The way you are fixing it
>> is by verifying by calling mremap() with MAP_FIXED_NOREPLACE
>> flag and check for EEXIST.
>>
> Yes, with the MREMAP_FIXED flag that the test uses, any previous
> mapping in the address range of the remap region gets unmapped.
> Yes, fixing this issue by calling mmap() with MAP_FIXED_NOREPLACE
> flag and checking for EEXIST.
> 
>> What happens when max address is reached?
>>
> That is covered by the check if (addr > ULLONG_MAX - region size)
> in the remap_region_valid() function.
>> Same comment on # of chars per line in commit log. Also
>>
>>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>> ---
>>>   tools/testing/selftests/vm/mremap_test.c | 36 ++++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c
>>> index 58600fee4b81..98e9cff34aa7 100644
>>> --- a/tools/testing/selftests/vm/mremap_test.c
>>> +++ b/tools/testing/selftests/vm/mremap_test.c
>>> @@ -10,6 +10,7 @@
>>>   #include <string.h>
>>>   #include <sys/mman.h>
>>>   #include <time.h>
>>> +#include <limits.h>
>>>     #include "../kselftest.h"
>>>   @@ -65,6 +66,34 @@ enum {
>>>       .expect_failure = should_fail                \
>>>   }
>>>   +/*
>>> + * Returns 0 if the requested remap region overlaps with an
>>> + * existing mapping (e.g text, stack) else returns 1.
>>> + */
>>> +static int remap_region_valid(void *addr, unsigned long long size)
>>
>> This returns bool 0 (false) 1 (true)
>>
>> Please name the routine - is_remap_region_valid() and change it to
>> return bool.
>>
>>> +{
>>> +    void *remap_addr = NULL;
>>> +    int ret = 1;
>>> +
>>> +    if ((unsigned long long) addr > ULLONG_MAX - size) {
>>> +        ksft_print_msg("Can't find a valid region to remap to\n");
>>
>> Change it to "Couldn't" - also this message doesn't look right. We hav't
>> looked for valid region yet and it just exceeds the limits?
>>
> Because this function is called in a loop in remap_region() and addr is being
> incremented continuously, we could enter this function with addr high enough that
> another increment would cause overflow.
>>

What this means is we could see lots of these messages that just say
"Can't find" without any other information such as the "addr"?

We probably don't want this message printed in a loop and print from
caller after the while loop ends without finding valid mapping.

>>> +        exit(KSFT_SKIP);> +    }
>>> +
>>> +    /* Use MAP_FIXED_NOREPLACE flag to ensure region is not mapped */
>>> +    remap_addr = mmap(addr, size, PROT_READ | PROT_WRITE,
>>> +            MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED,
>>> +            -1, 0);
>>
>> Alignment should match open parenthesis here and in other places. Makes it
>> easier to read the code.
>>
>>> +    if (remap_addr == MAP_FAILED) {
>>> +        if (errno == EEXIST)
>>> +            ret = 0;
>>> +    } else {
>>> +        munmap(remap_addr, size);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   /* Returns mmap_min_addr sysctl */
>>>   static unsigned long long get_mmap_min_addr(void)
>>>   {
>>> @@ -180,6 +209,13 @@ static long long remap_region(struct config c, unsigned int threshold_mb,
>>>       if (!((unsigned long long) addr & c.dest_alignment))
>>>           addr = (void *) ((unsigned long long) addr | c.dest_alignment);
>>>   +    /* Don't destroy existing mappings unless expected to overlap */
>>> +    while (!remap_region_valid(addr, c.region_size)) {
>>> +        if (c.overlapping)
>>> +            break;
>>> +        addr += c.src_alignment;
>>> +    }
>>> +

Here instead of in the loop.

>>>       clock_gettime(CLOCK_MONOTONIC, &t_start);
>>>       dest_addr = mremap(src_addr, c.region_size, c.region_size,
>>>               MREMAP_MAYMOVE|MREMAP_FIXED, (char *) addr);
>>>
>>

thanks,
-- Shuah