[RFC PATCH 0/5] mm: Fix mremap behavior when using addr hints

[RFC PATCH 0/5] mm: Fix mremap behavior when using addr hints

[Brian Geffon]

When MREMAP_DONTUNMAP was added in e346b38 ("mm/mremap: add
MREMAP_DONTUNMAP to mremap()") we inadvertently allows for an
address hint to be specified when not using MREMAP_FIXED. This is because
mremap(2) with MREMAP_DONTUNMAP it shares a code path with MREMAP_FIXED
in mremap_to(), which means this function can be called in 3 different
scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP
only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only
MREMAP_DONTUNMAP the new_addr is passed to get_unmapped_area() which
behaves like a hint as it does with mmap(2).

Glibc now expects this behavior so it would seem it probably cannot be
removed [1]. Additionally, as Jann Horn pointed out Debian apparently
has tests for this odd undocumented behavior [2].

This series attempts to reconcile this situation.

Patch 1: Will fix the checks that should only apply to the MREMAP_FIXED
case, so that they don't apply to just MREMAP_MAYMOVE.
Patch 2: Addresses an actual bug where we can allow a hint which is
lower than the mmap_min_addr.
Patch 3: Since we're stuck with this behavior we might as well support
it for mremap(2) when MREMAP_MAYMOVE is specified.
Patch 4: Self tests for patch 1
Patch 5: Self tests for patch 3

I will mail man page updates once we finalize on the behavior for
mremap(2).

1. https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206
2. https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818

Brian Geffon (5):
  mm: mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP
  mm: mremap: Use round_hint_to_min() for new_addr hints
  mm: mremap: Allow new_addr to be specified as a hint
  selftests: mm: Add a new MREMAP_DONTUNMAP self test
  selftests: mm: Add selftest for new_addr hint with MREMAP_MAYMOVE.

 include/linux/mm_inline.h                     |  14 +++
 mm/mmap.c                                     |  13 --
 mm/mremap.c                                   |  34 ++++--
 tools/testing/selftests/mm/mremap_dontunmap.c |  41 ++++++-
 tools/testing/selftests/mm/mremap_test.c      | 113 +++++++++++++++++-
 5 files changed, 189 insertions(+), 26 deletions(-)

-- 
2.47.0.338.g60cca15819-goog

[RFC PATCH 1/5] mm: mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP

[Brian Geffon]

Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
and MREMAP_DONTUNMAP which are called from mremap(). In the case of
MREMAP_FIXED we must validate the new_addr to ensure that the new
address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
a new address is specified as a hint, just like it would be in the
case of mmap. In this second case we don't need to perform any checks
because get_unmapped_area() will align new_addr, just like it would in
the case of mmap.

This patch only fixes the behavior that was inadvertently added
with MREMAP_DONTUNMAP.

v2:
  - Addressed comment from Marco Vanotti to consolidate these checks
    into existing MREMAP_FIXED blocks.

Signed-off-by: Brian Geffon <bgeffon@google.com>
Reported-by: Marco Vanotti <mvanotti@google.com>
---
 mm/mremap.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 60473413836b..62aec72bbe42 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -912,16 +912,6 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 	unsigned long ret;
 	unsigned long map_flags = 0;
 
-	if (offset_in_page(new_addr))
-		return -EINVAL;
-
-	if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
-		return -EINVAL;
-
-	/* Ensure the old/new locations do not overlap */
-	if (addr + old_len > new_addr && new_addr + new_len > addr)
-		return -EINVAL;
-
 	/*
 	 * move_vma() need us to stay 4 maps below the threshold, otherwise
 	 * it will bail out at the very beginning.
@@ -940,6 +930,25 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 		return -ENOMEM;
 
 	if (flags & MREMAP_FIXED) {
+		/*
+		 * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
+		 * and MREMAP_DONTUNMAP which are called from mremap(). In the case of
+		 * MREMAP_FIXED we must validate the new_addr to ensure that the new
+		 * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
+		 * a new address is specified as a hint, just like it would be in the
+		 * case of mmap. In this second case we don't need to perform any checks
+		 * because get_unmapped_area() will align new_addr, just like it would in
+		 * the case of mmap.
+		 */
+		if (offset_in_page(new_addr))
+			return -EINVAL;
+
+		if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
+			return -EINVAL;
+
+		/* Ensure the old/new locations do not overlap */
+		if (addr + old_len > new_addr && new_addr + new_len > addr)
+			return -EINVAL;
 		/*
 		 * In mremap_to().
 		 * VMA is moved to dst address, and munmap dst first.
-- 
2.47.0.338.g60cca15819-goog

[RFC PATCH 2/5] mm: mremap: Use round_hint_to_min() for new_addr hints

[Brian Geffon]

MREMAP_DONTUNMAP allows for a get_unmapped_area() hint
when used without MREMAP_FIXED. This was behavior was introduced
inadvertently and should be rounded to the mmap_min_address like
in mmap(2).

Signed-off-by: Brian Geffon <bgeffon@google.com>
---
 include/linux/mm_inline.h | 14 ++++++++++++++
 mm/mmap.c                 | 13 -------------
 mm/mremap.c               |  3 +++
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index 1b6a917fffa4..863143ec5bb0 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -9,6 +9,7 @@
 #include <linux/string.h>
 #include <linux/userfaultfd_k.h>
 #include <linux/swapops.h>
+#include <linux/security.h>
 
 /**
  * folio_is_file_lru - Should the folio be on a file LRU or anon LRU?
@@ -613,4 +614,17 @@ static inline bool vma_has_recency(struct vm_area_struct *vma)
 	return true;
 }
 
+/*
+ * If a hint addr is less than mmap_min_addr change hint to be as
+ * low as possible but still greater than mmap_min_addr
+ */
+static inline unsigned long round_hint_to_min(unsigned long hint)
+{
+	hint &= PAGE_MASK;
+	if (((void *)hint != NULL) &&
+	    (hint < mmap_min_addr))
+		return PAGE_ALIGN(mmap_min_addr);
+	return hint;
+}
+
 #endif
diff --git a/mm/mmap.c b/mm/mmap.c
index d32b7e701058..04952ac21d58 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -215,19 +215,6 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
 	return origbrk;
 }
 
-/*
- * If a hint addr is less than mmap_min_addr change hint to be as
- * low as possible but still greater than mmap_min_addr
- */
-static inline unsigned long round_hint_to_min(unsigned long hint)
-{
-	hint &= PAGE_MASK;
-	if (((void *)hint != NULL) &&
-	    (hint < mmap_min_addr))
-		return PAGE_ALIGN(mmap_min_addr);
-	return hint;
-}
-
 bool mlock_future_ok(struct mm_struct *mm, unsigned long flags,
 			unsigned long bytes)
 {
diff --git a/mm/mremap.c b/mm/mremap.c
index 62aec72bbe42..fdc1b0f1b38e 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1109,6 +1109,9 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 			goto out;
 	}
 
+	if (!(flags & MREMAP_FIXED))
+		new_addr = round_hint_to_min(new_addr);
+
 	if (flags & (MREMAP_FIXED | MREMAP_DONTUNMAP)) {
 		ret = mremap_to(addr, old_len, new_addr, new_len,
 				&locked, flags, &uf, &uf_unmap_early,
-- 
2.47.0.338.g60cca15819-goog

[RFC PATCH 3/5] mm: mremap: Allow new_addr to be specified as a hint

[Brian Geffon]

When using MREMAP_MAYMOVE previously the new_addr was ignored unless
the user specified MREMAP_FIXED. This change will allow it to be
used as a hint in that situation similar to how mmap(2) behaves.

get_unmapped_area() will handle page aligning the new address hint.

Signed-off-by: Brian Geffon <bgeffon@google.com>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index fdc1b0f1b38e..1d2522fba0ef 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1205,7 +1205,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 		if (vma->vm_flags & VM_MAYSHARE)
 			map_flags |= MAP_SHARED;
 
-		new_addr = get_unmapped_area(vma->vm_file, 0, new_len,
+		new_addr = get_unmapped_area(vma->vm_file, new_addr, new_len,
 					vma->vm_pgoff +
 					((addr - vma->vm_start) >> PAGE_SHIFT),
 					map_flags);
-- 
2.47.0.338.g60cca15819-goog

[RFC PATCH 4/5] selftests: mm: Add a new MREMAP_DONTUNMAP self test

[Brian Geffon]

Add a new selftest which validates that a new_addr as a hint behaves
in the same way as mmap.

Signed-off-by: Brian Geffon <bgeffon@google.com>
---
 tools/testing/selftests/mm/mremap_dontunmap.c | 41 ++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mremap_dontunmap.c b/tools/testing/selftests/mm/mremap_dontunmap.c
index 1d75084b9ca5..ccce97c68019 100644
--- a/tools/testing/selftests/mm/mremap_dontunmap.c
+++ b/tools/testing/selftests/mm/mremap_dontunmap.c
@@ -224,6 +224,44 @@ static void mremap_dontunmap_simple_fixed()
 	ksft_test_result_pass("%s\n", __func__);
 }
 
+// This test validates MREMAP_DONTUNMAP using a newaddr hint without
+// MREMAP_FIXED.
+static void mremap_dontunmap_simple_newaddr_hint()
+{
+	unsigned long num_pages = 5;
+
+	// This dest hint is intentionally not aligned.
+	void *new_addr_hint = (void*)0x999900010;
+
+	void *source_mapping =
+	    mmap(NULL, num_pages * page_size, PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	BUG_ON(source_mapping == MAP_FAILED, "mmap");
+	memset(source_mapping, 'a', num_pages * page_size);
+
+	void *remapped_mapping =
+	    mremap(source_mapping, num_pages * page_size, num_pages * page_size,
+		   MREMAP_DONTUNMAP | MREMAP_MAYMOVE,
+		   new_addr_hint);
+	BUG_ON(remapped_mapping == MAP_FAILED, "mremap");
+
+	// And the source mapping will have had its ptes dropped.
+	BUG_ON(check_region_contains_byte
+	       (source_mapping, num_pages * page_size, 0) != 0,
+	       "source should have no ptes");
+
+	// And the remapped area will be filled with 'a's.
+	BUG_ON(check_region_contains_byte
+	       (remapped_mapping, num_pages * page_size, 'a') != 0,
+	       "dest should have remapped content");
+
+	BUG_ON(munmap(source_mapping, num_pages * page_size) == -1,
+	       "unable to unmap source mapping");
+	BUG_ON(munmap(remapped_mapping, num_pages * page_size) == -1,
+	       "unable to unmap source mapping");
+	ksft_test_result_pass("%s\n", __func__);
+}
+
 // This test validates that we can MREMAP_DONTUNMAP for a portion of an
 // existing mapping.
 static void mremap_dontunmap_partial_mapping()
@@ -348,7 +386,7 @@ int main(void)
 		ksft_finished();
 	}
 
-	ksft_set_plan(5);
+	ksft_set_plan(6);
 
 	// Keep a page sized buffer around for when we need it.
 	page_buffer =
@@ -359,6 +397,7 @@ int main(void)
 	mremap_dontunmap_simple();
 	mremap_dontunmap_simple_shmem();
 	mremap_dontunmap_simple_fixed();
+	mremap_dontunmap_simple_newaddr_hint();
 	mremap_dontunmap_partial_mapping();
 	mremap_dontunmap_partial_mapping_overwrite();
 
-- 
2.47.0.338.g60cca15819-goog

[RFC PATCH 5/5] selftests: mm: Add selftest for new_addr hint with MREMAP_MAYMOVE.

[Brian Geffon]

This selftest tries to validate that a hint will be used with
MREMAP_MAYMOVE. Given it's just a hint we try to structure the test
in a way where it will succeed.

To ensure we can validate the behavior we create a single mapping
and split it by unmapping the middle and we'll use that as a hint
for MREMAP_MAYMOVE without using MREMAP_FIXED.

Signed-off-by: Brian Geffon <bgeffon@google.com>
---
 tools/testing/selftests/mm/mremap_test.c | 113 ++++++++++++++++++++++-
 1 file changed, 112 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c
index 5a3a9bcba640..42ed869e2e01 100644
--- a/tools/testing/selftests/mm/mremap_test.c
+++ b/tools/testing/selftests/mm/mremap_test.c
@@ -8,6 +8,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
+#include <syscall.h>
 #include <sys/mman.h>
 #include <time.h>
 #include <stdbool.h>
@@ -270,6 +271,115 @@ static void mremap_expand_merge(FILE *maps_fp, unsigned long page_size)
 		ksft_test_result_fail("%s\n", test_name);
 }
 
+/*
+ * This test validates that mremap(2) with MREMAP_MAYMOVE uses the new
+ * address as a hint.
+ */
+static void mremap_maymove_hint(FILE *maps_fp, unsigned long page_size)
+{
+	char *test_name = "mremap MAY_MOVE with hint";
+	void *mapping_a, *mapping_b, *mapping_c, *remapped, *hint;
+	intptr_t base_map_addr = 0x8FF00000;
+
+#if !defined(__i386__) && !defined(__x86_64__)
+	/*
+	 * This test is written with knowledge about the architecture specific behavior of
+	 * get_unmapped_area(). For that reason this specific test is only applicable to x86.
+	 */
+	ksft_test_result_skip("%s\n", test_name);
+	return;
+#endif
+
+	/*
+	 * To validate the behavior we'll use the following layout:
+	 *
+	 * | mapping a |                   |   mapping b    |   mapping c   |
+	 * | 1 page    | 10 pages unmapped | 2 pages mapped | 1 page mapped |
+	 *
+	 * To guarantee we can get this layout we'll do a single mmap and then
+	 * munmap and mprotect accordingly, this will prevent the test from being
+	 * flaky.
+	 *
+	 * We'll attempt to resize mapping b to 3 pages using MAYMOVE, because
+	 * mapping c is beyond it it'll have to be moved. We will use mapping a
+	 * as the hint to validate it lands just beyond it. The final result:
+	 *
+	 * | mapping a |                  | mapping b      |                  |   mapping c   |
+	 * | 1 page    | 2 pages unmapped | 3 pages mapped | 7 pages unampped | 1 page mapped |
+	 *
+	 */
+	mapping_a = mmap((void*)base_map_addr, 14 * page_size, PROT_READ | PROT_WRITE,
+		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	if (mapping_a == MAP_FAILED) {
+		ksft_print_msg("mmap failed: %s\n", strerror(errno));
+		goto out_fail;
+	}
+
+	mapping_b = (void*)((intptr_t)mapping_a + 11*page_size);
+	mapping_c = (void*)((intptr_t)mapping_a + 13*page_size);
+
+	/* unmap the 10 pages after mapping a */
+	munmap((void*)((intptr_t)(mapping_a) + page_size), 10*page_size);
+
+	/* make mapping a and c PROT_NONE to complete the vma splitting */
+	mprotect(mapping_a, page_size, PROT_NONE);
+	mprotect(mapping_c, page_size, PROT_NONE);
+
+	/*
+	 * Validate the split: mapping a, b, and c are mapped with a gap after 'a'.
+	 */
+	if (!is_range_mapped(maps_fp, (unsigned long)mapping_a,
+				  (unsigned long)(mapping_a + page_size))) {
+		ksft_print_msg("mapping 'a' was not mapped at %p\n", mapping_a);
+		goto out_fail;
+	}
+
+	if (is_range_mapped(maps_fp, (unsigned long)mapping_a + page_size,
+				  (unsigned long)mapping_a + 10*page_size)) {
+		ksft_print_msg("unmapped area after mapping 'a' not found\n");
+		goto out_fail;
+	}
+
+	if (!is_range_mapped(maps_fp, (unsigned long)mapping_b,
+				  (unsigned long)(mapping_b + 2*page_size))) {
+		ksft_print_msg("mapping 'b' was not mapped at %p\n", mapping_b);
+		goto out_fail;
+	}
+
+	if (!is_range_mapped(maps_fp, (unsigned long)mapping_c,
+				  (unsigned long)(mapping_c + page_size))) {
+		ksft_print_msg("mapping 'c' was not mapped at %p\n", mapping_c);
+		goto out_fail;
+	}
+
+	/*
+	 * Now try to mremap mapping 'b' using a hint, it will be increased in size
+	 * so that the VMA must be moved. Bypass the glibc wrapper of mremap(2) becuase
+	 * it will attempt to 0 the hint unless MREMAP_DONTUNMAP is set, for reference:
+	 * https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=6c40cb0e9f893d49dc7caee580a055de53562206;hp=54252394c25ddf0062e288d4a6ab7a885f8ae009
+	 */
+	hint = (void*)((intptr_t)mapping_a + 3*page_size);
+	remapped = (void*)syscall(SYS_mremap, mapping_b, 2*page_size, 3*page_size, MREMAP_MAYMOVE, hint);
+	if ((intptr_t)remapped != (intptr_t)hint) {
+		if (remapped == MAP_FAILED)
+			ksft_print_msg("remap of 'b' failed %s\n", strerror(errno));
+		else
+			ksft_print_msg("mapping 'b' was unexpectedly remapped from %p to %p. expected: %p\n",
+				mapping_b, remapped, hint);
+		goto out_fail;
+	}
+
+	munmap(mapping_a, page_size);
+	munmap(remapped, 3 * page_size);
+	munmap(mapping_c, page_size);
+	ksft_test_result_pass("%s\n", test_name);
+	return;
+
+out_fail:
+	ksft_test_result_fail("%s\n", test_name);
+}
+
 /*
  * Similar to mremap_expand_merge() except instead of removing the middle page,
  * we remove the last then attempt to remap offset from the second page. This
@@ -720,7 +830,7 @@ int main(int argc, char **argv)
 	char *rand_addr;
 	size_t rand_size;
 	int num_expand_tests = 2;
-	int num_misc_tests = 2;
+	int num_misc_tests = 3;
 	struct test test_cases[MAX_TEST] = {};
 	struct test perf_test_cases[MAX_PERF_TEST];
 	int page_size;
@@ -842,6 +952,7 @@ int main(int argc, char **argv)
 
 	mremap_expand_merge(maps_fp, page_size);
 	mremap_expand_merge_offset(maps_fp, page_size);
+	mremap_maymove_hint(maps_fp, page_size);
 
 	fclose(maps_fp);
 
-- 
2.47.0.338.g60cca15819-goog

Re: [RFC PATCH 1/5] mm: mremap: Fix new_addr being used as a hint with MREMAP_DONTUNMAP

[Marco Vanotti]

[-- Attachment #1: Type: text/plain, Size: 3386 bytes --]

On Tue, Dec 10, 2024 at 6:31 PM Brian Geffon <bgeffon@google.com> wrote:
>
> Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
> and MREMAP_DONTUNMAP which are called from mremap(). In the case of
> MREMAP_FIXED we must validate the new_addr to ensure that the new
> address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
> a new address is specified as a hint, just like it would be in the
> case of mmap. In this second case we don't need to perform any checks
> because get_unmapped_area() will align new_addr, just like it would in
> the case of mmap.
>
> This patch only fixes the behavior that was inadvertently added
> with MREMAP_DONTUNMAP.
>
> v2:
>   - Addressed comment from Marco Vanotti to consolidate these checks
>     into existing MREMAP_FIXED blocks.
>
> Signed-off-by: Brian Geffon <bgeffon@google.com>
> Reported-by: Marco Vanotti <mvanotti@google.com>
Reviewed-By: Marco Vanotti <mvanotti@google.com>
> ---
>  mm/mremap.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 60473413836b..62aec72bbe42 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -912,16 +912,6 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>         unsigned long ret;
>         unsigned long map_flags = 0;
>
> -       if (offset_in_page(new_addr))
> -               return -EINVAL;
> -
> -       if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> -               return -EINVAL;
> -
> -       /* Ensure the old/new locations do not overlap */
> -       if (addr + old_len > new_addr && new_addr + new_len > addr)
> -               return -EINVAL;
> -
>         /*
>          * move_vma() need us to stay 4 maps below the threshold, otherwise
>          * it will bail out at the very beginning.
> @@ -940,6 +930,25 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>                 return -ENOMEM;
>
>         if (flags & MREMAP_FIXED) {
> +               /*
> +                * Two non-mutually exclusive paths can land in mremap_to, MREMAP_FIXED
> +                * and MREMAP_DONTUNMAP which are called from mremap(). In the case of
> +                * MREMAP_FIXED we must validate the new_addr to ensure that the new
> +                * address is valid. In the case of MREMAP_DONTUNMAP without MREMAP_FIXED
> +                * a new address is specified as a hint, just like it would be in the
> +                * case of mmap. In this second case we don't need to perform any checks
> +                * because get_unmapped_area() will align new_addr, just like it would in
> +                * the case of mmap.
> +                */
> +               if (offset_in_page(new_addr))
> +                       return -EINVAL;
> +
> +               if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
> +                       return -EINVAL;
> +
> +               /* Ensure the old/new locations do not overlap */
> +               if (addr + old_len > new_addr && new_addr + new_len > addr)
> +                       return -EINVAL;
>                 /*
>                  * In mremap_to().
>                  * VMA is moved to dst address, and munmap dst first.
> --
> 2.47.0.338.g60cca15819-goog
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

Re: [RFC PATCH 3/5] mm: mremap: Allow new_addr to be specified as a hint

[Brian Geffon]

On Tue, Dec 10, 2024 at 1:31 PM Brian Geffon <bgeffon@google.com> wrote:
>
> When using MREMAP_MAYMOVE previously the new_addr was ignored unless
> the user specified MREMAP_FIXED. This change will allow it to be
> used as a hint in that situation similar to how mmap(2) behaves.
>
> get_unmapped_area() will handle page aligning the new address hint.
>
> Signed-off-by: Brian Geffon <bgeffon@google.com>
> ---
>  mm/mremap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index fdc1b0f1b38e..1d2522fba0ef 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1205,7 +1205,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>                 if (vma->vm_flags & VM_MAYSHARE)
>                         map_flags |= MAP_SHARED;
>
> -               new_addr = get_unmapped_area(vma->vm_file, 0, new_len,
> +               new_addr = get_unmapped_area(vma->vm_file, new_addr, new_len,

Marco raised the concern that since glibc [1] handles new_addr as a
variadic argument, existing call sites which used only MREMAP_MAYMOVE
might end up passing garbage to the glibc wrapper for new_addr. I
checked and it turns out that musl does the same [2]. So I'm not sure
how this could ever be safely implemented? Worst case scenario we're
leaking stack data as a hint to mremap, yikes.

1. https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/mremap.c
2. https://git.musl-libc.org/cgit/musl/tree/src/mman/mremap.c

>                                         vma->vm_pgoff +
>                                         ((addr - vma->vm_start) >> PAGE_SHIFT),
>                                         map_flags);
> --
> 2.47.0.338.g60cca15819-goog
>