[PATCH 0/6] mm/mremap: report a failed split before moving anything

[PATCH 0/6] mm/mremap: report a failed split before moving anything

[Josef Bacik]

Hello,

I went through the callers of __split_vma() to see who can't cope with
it failing, and mremap() is the bad one.  If you move part of a VMA we
don't split it until the very end, when we unmap the old range.  By then
the page tables have moved and there's nothing to go back to, so we fix
up the accounting and return success with the old range still mapped.

Lorenzo recently added a way to split ahead of the move for the mlock()
+ MREMAP_DONTUNMAP case.  This series uses that for every move that
unmaps its source, so a split that fails is reported before we've done
anything.

Doing that turned up a real bug in hugetlb.  We take the reservation map
away from the whole old VMA before the part that's moving is split off,
so the parts that stay never give their reservations back.  That fix is
the first patch so it can go to stable by itself, and the second patch
is a selftest for it.

The next two fix the accounting for when the unmap at the end does fail,
we either charged memory that was never accounted or lost track of the
charge for the whole VMA.  That gets a lot harder to hit with the last
patch but it's still possible.  There's also a test for an old size of
0, which nothing covered.

I tested this with the mm selftests and with a loop that fails each
allocation inside mremap() in turn and checks that we either moved or
didn't.  The numbers for moving part of a mapping don't change.  Thanks,

Josef

---
Josef Bacik (6):
      mm/mremap: fix hugetlb reservation leak on partial move
      selftests/mm: test a partial mremap() of hugetlb memory
      mm/mremap: reaccount on unmap failure only if accounted
      mm/mremap: restore VM_ACCOUNT if the unmap fails
      selftests/mm: test mremap() with an old size of 0
      mm/mremap: split first so a failed split is reported

 mm/mremap.c                                 | 55 +++++++++++++++++++++++------
 tools/testing/selftests/mm/hugetlb-mremap.c | 52 +++++++++++++++++++++++++--
 tools/testing/selftests/mm/mremap_test.c    | 52 ++++++++++++++++++++++++++-
 3 files changed, 145 insertions(+), 14 deletions(-)
---
base-commit: cd1d0b147afa81f80927f9711e53401703964217
change-id: 20261030-b4-mremap-pre-split-13a3d54d6dbd

[PATCH 1/6] mm/mremap: fix hugetlb reservation leak on partial move

[Josef Bacik]

Moving a part of a private hugetlb mapping with mremap() leaks the huge
page reservations of the parts that stay. They are not returned when the
mapping is unmapped, when the process exits or when the pool is resized.

fixup_hugetlb_reservations() takes the reservation map away from the old
VMA while that VMA still spans its whole range, because the moved part is
only split off by the unmap at the end of the move. hugetlb_vm_op_close()
then finds no map in the parts that remain and returns nothing for them:

    mmap() of 4 pages, MAP_PRIVATE | MAP_HUGETLB    HugePages_Rsvd: 4
    mremap() of the second page to a new address    HugePages_Rsvd: 4
    munmap() of all of it                           HugePages_Rsvd: 3

The source is unmapped in this case, so the number of VMAs ends up where it
did before and the check against sysctl_max_map_count stays as it is.

Fix by splitting a hugetlb VMA in prep_move_vma() before a part of it is
moved, as commit 597e4b9ab48a ("mm/mremap: fix locked_vm leak by splitting
VMA for MREMAP_DONTUNMAP") does for an mlock()'d VMA. A backport needs that
commit, which is tagged for stable itself.

Fixes: 550a7d60bd5e ("mm, hugepages: add mremap() support for hugepage backed vma")
Cc: <stable@vger.kernel.org>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 mm/mremap.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 0d06d7f37f19..cf746bafdee4 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1038,7 +1038,7 @@ static void vrm_stat_account(struct vma_remap_struct *vrm,
 }
 
 static bool __check_map_count_against_split(struct mm_struct *mm,
-					    bool pre_split,
+					    bool source_stays,
 					    bool before_unmaps)
 {
 	const int sys_map_count = get_sysctl_max_map_count();
@@ -1092,14 +1092,15 @@ static bool __check_map_count_against_split(struct mm_struct *mm,
 	 */
 	map_count += 2;
 
-	/* If pre-split, the -1 observed above doesn't apply. */
-	if (pre_split)
+	/* If the split off source stays, the -1 observed above doesn't apply. */
+	if (source_stays)
 		map_count++;
 
 	return map_count <= sys_map_count;
 }
 
-static bool needs_pre_split(struct vma_remap_struct *vrm)
+/* Is the source range split off its VMA, to then stay mapped? */
+static bool split_source_stays(struct vma_remap_struct *vrm)
 {
 	/*
 	 * An MREMAP_DONTUNMAP of a mlock()'d VMA needs to unlock the
@@ -1109,11 +1110,23 @@ static bool needs_pre_split(struct vma_remap_struct *vrm)
 		vma_test(vrm->vma, VMA_LOCKED_BIT);
 }
 
+static bool needs_pre_split(struct vma_remap_struct *vrm)
+{
+	/*
+	 * fixup_hugetlb_reservations() takes the reservations away from the
+	 * source VMA.  The parts of it that do not move have to keep theirs.
+	 */
+	if (vma_is_hugetlb(vrm->vma))
+		return true;
+
+	return split_source_stays(vrm);
+}
+
 /* Do we violate the map count limit if we split VMAs when moving the VMA? */
 static bool check_map_count_against_split(struct vma_remap_struct *vrm)
 {
 	return __check_map_count_against_split(current->mm,
-		needs_pre_split(vrm), /*before_unmaps=*/false);
+		split_source_stays(vrm), /*before_unmaps=*/false);
 }
 
 /* Do we violate the map count limit if we split VMAs prior to early unmaps? */
@@ -1162,10 +1175,7 @@ static unsigned long prep_move_vma(struct vma_remap_struct *vrm)
 	if (err)
 		return err;
 
-	/*
-	 * To account mlock()'d pages correctly in the MREMAP_DONTUNMAP
-	 * case perform any split ahead of time for an mlock()'d VMA.
-	 */
+	/* Perform any split ahead of time if the source VMA has to be distinct. */
 	if (needs_pre_split(vrm)) {
 		VMA_ITERATOR(vmi, vma->vm_mm, old_addr);
 

-- 
2.55.0

[PATCH 2/6] selftests/mm: test a partial mremap() of hugetlb memory

[Josef Bacik]

hugetlb-mremap only moves whole mappings, so it did not catch the
reservation leak of a partial move.

Map four private huge pages without touching them, move the second one,
unmap everything, and check that the number of reserved huge pages is back
where it was.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tools/testing/selftests/mm/hugetlb-mremap.c | 52 +++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/hugetlb-mremap.c b/tools/testing/selftests/mm/hugetlb-mremap.c
index 9b724af66e93..9f9d6905cd6d 100644
--- a/tools/testing/selftests/mm/hugetlb-mremap.c
+++ b/tools/testing/selftests/mm/hugetlb-mremap.c
@@ -95,6 +95,51 @@ static void register_region_with_uffd(char *addr, size_t len)
 	ksft_print_msg("Registered memory at address %p with userfaultfd\n", addr);
 }
 
+/*
+ * Move one page out of the middle of a private mapping that has not been
+ * touched, so that every page of it still holds its reservation.  Once all of
+ * it is unmapped, all of the reservations have to be back in the pool.
+ */
+static void test_partial_move(unsigned long hugepage_size)
+{
+	const int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB;
+	const int prot = PROT_READ | PROT_WRITE;
+	const unsigned long nr = 4;
+	unsigned long resv = hugetlb_nr_resv_pages(hugepage_size);
+	unsigned long resv_after;
+	char *src, *dst, *addr;
+
+	/* The pages of the test before this one can still be in use. */
+	if (!hugetlb_setup_default(resv + nr + 1)) {
+		ksft_test_result_skip("Partial move: not enough free huge pages\n");
+		return;
+	}
+
+	src = mmap(NULL, nr * hugepage_size, prot, flags, -1, 0);
+	if (src == MAP_FAILED)
+		ksft_exit_fail_msg("mmap src: %s\n", strerror(errno));
+
+	/* The destination has to be hugetlb-backed, mremap() replaces it. */
+	dst = mmap(NULL, hugepage_size, prot, flags, -1, 0);
+	if (dst == MAP_FAILED)
+		ksft_exit_fail_msg("mmap dst: %s\n", strerror(errno));
+
+	addr = mremap(src + hugepage_size, hugepage_size, hugepage_size,
+		      MREMAP_MAYMOVE | MREMAP_FIXED, dst);
+	if (addr == MAP_FAILED)
+		ksft_exit_fail_msg("partial mremap: %s\n", strerror(errno));
+
+	munmap(addr, hugepage_size);
+	munmap(src, nr * hugepage_size);
+
+	resv_after = hugetlb_nr_resv_pages(hugepage_size);
+	if (resv_after != resv)
+		ksft_print_msg("Reserved huge pages: %lu before, %lu after\n",
+			       resv, resv_after);
+	ksft_test_result(resv_after == resv,
+			 "Partial move returns all reservations\n");
+}
+
 int main(int argc, char *argv[])
 {
 	unsigned long hugepage_size;
@@ -102,7 +147,7 @@ int main(int argc, char *argv[])
 	size_t length = 0;
 
 	ksft_print_header();
-	ksft_set_plan(1);
+	ksft_set_plan(2);
 
 	if (argc >= 2 && !strcmp(argv[1], "-h"))
 		ksft_exit_fail_msg("Usage: %s [length_in_MB]\n", argv[0]);
@@ -170,5 +215,8 @@ int main(int argc, char *argv[])
 	close(fd);
 
 	ksft_test_result(!ret, "Read same data\n");
-	ksft_exit(!ret);
+
+	test_partial_move(hugepage_size);
+
+	ksft_finished();
 }

-- 
2.55.0

[PATCH 3/6] mm/mremap: reaccount on unmap failure only if accounted

[Josef Bacik]

When the unmap of the source range fails at the end of an mremap() move,
the length of the range is charged to vm_committed_as even if the VMA was
never accounted, for example a MAP_SHARED or MAP_NORESERVE mapping. Nothing
ever uncharges these pages, so each such failure permanently inflates the
committed memory count.

This was reproduced with fault injection: each failure on a shmem mapping
left Committed_AS higher by the size of the moved range after everything
was unmapped.

Fix by charging the range again only when accountable_move is set.

Fixes: b714ccb02a76 ("mm/mremap: complete refactor of move_vma()")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 mm/mremap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index cf746bafdee4..6f941aeb1b5f 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1247,7 +1247,8 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
 	vrm->vmi_needs_invalidate = true;
 	if (err) {
 		/* OOM: unable to split vma, just get accounts right */
-		vm_acct_memory(len >> PAGE_SHIFT);
+		if (accountable_move)
+			vm_acct_memory(len >> PAGE_SHIFT);
 		return;
 	}
 

-- 
2.55.0

[PATCH 4/6] mm/mremap: restore VM_ACCOUNT if the unmap fails

[Josef Bacik]

If the unmap at the end of an mremap() move fails, the source VMA stays
without VM_ACCOUNT. Neither munmap() nor exit_mmap() uncharges it then, so
the charge for all of its pages stays in vm_committed_as after the process
is gone.

unmap_source_vma() clears the flag on the whole source VMA before the unmap
and sets it again on the remaining parts afterwards, but its error path
returns without doing so.

This was reproduced with fault injection: each failure on an anonymous
mapping left Committed_AS higher by the size of the whole VMA after
everything was unmapped.

Fix by setting VM_ACCOUNT again on everything in the range of the original
VMA. A failed unmap may have split the VMA already, so walk the range.

Fixes: 51df7bcb6151 ("mm/mremap: account memory on do_munmap() failure")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 mm/mremap.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 6f941aeb1b5f..256eac721a07 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1246,9 +1246,20 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
 	vrm->vma = NULL; /* Invalidated. */
 	vrm->vmi_needs_invalidate = true;
 	if (err) {
-		/* OOM: unable to split vma, just get accounts right */
-		if (accountable_move)
+		/*
+		 * OOM: unable to split vma, just get accounts right. Both the
+		 * old and the new range remain mapped, so charge the range a
+		 * second time, and reinstate the flag on all that is left of
+		 * the VMA so the charge is returned when it gets unmapped.
+		 */
+		if (accountable_move) {
 			vm_acct_memory(len >> PAGE_SHIFT);
+			vma_iter_set(&vmi, vm_start);
+			for_each_vma_range(vmi, vma, vm_end) {
+				vma_start_write(vma);
+				vma_set_flags(vma, VMA_ACCOUNT_BIT);
+			}
+		}
 		return;
 	}
 

-- 
2.55.0

[PATCH 5/6] selftests/mm: test mremap() with an old size of 0

[Josef Bacik]

mremap() with an old size of 0 duplicates a shared mapping, and mremap_test
has no case for it.

Duplicate a shared anonymous mapping, once from its start and once from the
middle. Check that the source and the duplicate are mapped by one VMA each
and that they show the same pages.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tools/testing/selftests/mm/mremap_test.c | 52 +++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c
index 31b484517bda..92960fff9537 100644
--- a/tools/testing/selftests/mm/mremap_test.c
+++ b/tools/testing/selftests/mm/mremap_test.c
@@ -603,6 +603,54 @@ static void mremap_shrink_multiple_vmas(unsigned long page_size,
 				      inplace ? " [inplace]" : "");
 }
 
+/*
+ * An old size of 0 duplicates a shared mapping: the pages show up at a second
+ * address, and the mapping they were taken from stays as it is.
+ */
+static void mremap_duplicate_shared(FILE *maps_fp, unsigned long page_size,
+				    unsigned long offset)
+{
+	char *test_name = "mremap duplicate shared mapping";
+	const size_t size = 3 * page_size;
+	const size_t dup_size = size - offset;
+	char *dup = MAP_FAILED;
+	bool success = false;
+	char *ptr;
+
+	ptr = mmap(NULL, size, PROT_READ | PROT_WRITE,
+		   MAP_SHARED | MAP_ANON, -1, 0);
+	if (ptr == MAP_FAILED) {
+		perror("mmap");
+		goto out;
+	}
+	memset(ptr, 'a', size);
+
+	dup = mremap(ptr + offset, 0, dup_size, MREMAP_MAYMOVE);
+	if (dup == MAP_FAILED) {
+		perror("mremap");
+		goto out_unmap;
+	}
+
+	/* Both are mapped, each by a single VMA. */
+	if (!is_ptr_mapped(maps_fp, ptr, size) ||
+	    !is_ptr_mapped(maps_fp, dup, dup_size))
+		goto out_unmap;
+
+	/* They show the same pages. */
+	ptr[offset] = 'b';
+	dup[dup_size - 1] = 'c';
+	success = dup[0] == 'b' && ptr[size - 1] == 'c' && ptr[0] != 'c';
+
+out_unmap:
+	if (dup != MAP_FAILED && munmap(dup, dup_size))
+		perror("munmap dup");
+	if (munmap(ptr, size))
+		perror("munmap src");
+out:
+	ksft_test_result(success, "%s%s\n", test_name,
+			 offset ? " [from the middle]" : "");
+}
+
 static void mremap_move_multiple_vmas_split(unsigned int pattern_seed,
 					    unsigned long page_size,
 					    bool dont_unmap)
@@ -1248,7 +1296,7 @@ int main(int argc, char **argv)
 	char *rand_addr;
 	size_t rand_size;
 	int num_expand_tests = 2;
-	int num_misc_tests = 9;
+	int num_misc_tests = 11;
 	struct test test_cases[MAX_TEST] = {};
 	struct test perf_test_cases[MAX_PERF_TEST];
 	int page_size;
@@ -1385,6 +1433,8 @@ int main(int argc, char **argv)
 	mremap_move_multiple_vmas_split(pattern_seed, page_size, /* dontunmap= */ false);
 	mremap_move_multiple_vmas_split(pattern_seed, page_size, /* dontunmap= */ true);
 	mremap_move_multi_invalid_vmas(maps_fp, page_size);
+	mremap_duplicate_shared(maps_fp, page_size, 0);
+	mremap_duplicate_shared(maps_fp, page_size, page_size);
 
 	fclose(maps_fp);
 

-- 
2.55.0

[PATCH 6/6] mm/mremap: split first so a failed split is reported

[Josef Bacik]

mremap() can return the new address although the old range is still mapped,
which leaves the caller with a VMA it does not know about.

To move a part of a VMA, that part has to be split off, and this is left to
the unmap at the end of the move. If the split fails there, the page tables
have moved already and there is nothing to go back to. unmap_source_vma()
returns void, so move_vma() reports success.

This was reproduced with fault injection, by failing each allocation of
such an mremap() in turn. Anonymous and shmem mappings hit it at 2 to 7 of
7 to 13 allocations, with numbers that differ from boot to boot, and no
longer hit it with this patch.

Fix by splitting the source VMA in prep_move_vma() whenever it is going to
be unmapped, so that a failed split is reported before anything has moved.
An old size of 0 unmaps nothing and does not split. If a later step fails,
the VMA stays split, as it does after a munmap() that fails half way. The
unmap at the end can still fail in its allocations, or if copy_vma() has
merged the range and it has to be split again.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 mm/mremap.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 256eac721a07..64f8e0895e77 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1112,11 +1112,20 @@ static bool split_source_stays(struct vma_remap_struct *vrm)
 
 static bool needs_pre_split(struct vma_remap_struct *vrm)
 {
+	/* This duplicates a shared mapping: no range to split off or unmap. */
+	if (!vrm->old_len)
+		return false;
+
 	/*
-	 * fixup_hugetlb_reservations() takes the reservations away from the
-	 * source VMA.  The parts of it that do not move have to keep theirs.
+	 * The source range is unmapped once its page tables have moved.  If a
+	 * split fails at that point there is nothing left to go back to, so
+	 * split while the failure can still be reported.
+	 *
+	 * hugetlb relies on this too: fixup_hugetlb_reservations() takes the
+	 * reservations away from the source VMA, and the parts of it that do
+	 * not move have to keep theirs.
 	 */
-	if (vma_is_hugetlb(vrm->vma))
+	if (!(vrm->flags & MREMAP_DONTUNMAP))
 		return true;
 
 	return split_source_stays(vrm);
@@ -1175,10 +1184,10 @@ static unsigned long prep_move_vma(struct vma_remap_struct *vrm)
 	if (err)
 		return err;
 
-	/* Perform any split ahead of time if the source VMA has to be distinct. */
 	if (needs_pre_split(vrm)) {
 		VMA_ITERATOR(vmi, vma->vm_mm, old_addr);
 
+		/* This leaves vma as the VMA that spans the range to move. */
 		if (split_before)
 			err = split_vma(&vmi, vma, old_addr, 1);
 		if (!err && split_after)
@@ -1247,10 +1256,12 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
 	vrm->vmi_needs_invalidate = true;
 	if (err) {
 		/*
-		 * OOM: unable to split vma, just get accounts right. Both the
-		 * old and the new range remain mapped, so charge the range a
-		 * second time, and reinstate the flag on all that is left of
-		 * the VMA so the charge is returned when it gets unmapped.
+		 * The unmap failed, just get accounts right. Both the old and
+		 * the new range remain mapped, so charge the range a second
+		 * time, and reinstate the flag on all that is left of the VMA
+		 * so the charge is returned when it gets unmapped.
+		 *
+		 * An old_len of 0 ends up here as well. It has nothing to unmap.
 		 */
 		if (accountable_move) {
 			vm_acct_memory(len >> PAGE_SHIFT);

-- 
2.55.0

Re: [PATCH 6/6] mm/mremap: split first so a failed split is reported

[Jose A. Perez de Azpillaga]

On 10/30/26 3:42 PM, Josef Bacik wrote:
> mremap() can return the new address although the old range is still mapped,
> which leaves the caller with a VMA it does not know about.
> 
> To move a part of a VMA, that part has to be split off, and this is left to
> the unmap at the end of the move. If the split fails there, the page tables
> have moved already and there is nothing to go back to. unmap_source_vma()
> returns void, so move_vma() reports success.
> 
> This was reproduced with fault injection, by failing each allocation of
> such an mremap() in turn. Anonymous and shmem mappings hit it at 2 to 7 of
> 7 to 13 allocations, with numbers that differ from boot to boot, and no
> longer hit it with this patch.
> 
> Fix by splitting the source VMA in prep_move_vma() whenever it is going to
> be unmapped, so that a failed split is reported before anything has moved.
> An old size of 0 unmaps nothing and does not split. If a later step fails,
> the VMA stays split, as it does after a munmap() that fails half way. The
> unmap at the end can still fail in its allocations, or if copy_vma() has
> merged the range and it has to be split again.
that later step isn't always an allocation failure though. a partial
mremap() of an aio ring still fails -EINVAL, but now leaves the ring
split in three. is that intended?

-- 
cheers,
jose a. p-a