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

From: Sidhartha Kumar
Date: Thu Apr 14 2022 - 17:39:25 EST



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@xxxxxxxxxx>
---
  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