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