[PATCH] RCU safety for vma maple tree walks

[PATCH] RCU safety for vma maple tree walks

[Andi Kleen]

I ran into the following scenario in a slightly modified kernel:

 1. vms_gather_munmap_vmas walks the unmap range and caches a maple node N
    in the maple iterator.
 2. It triggers a __split_vma to fix up a range and during that node N
    is queued for freeing with kfree_rcu
 3. There is another __split_vma that allocates memory and sleeps due to
    memory pressure.
 4. During the sleep the grace period expires and node N gets freed for
    real.
 5. The iterator still has node N cached
 6. When the iteration continues it accesses the freed node and KASAN
    trips:

BUG: KASAN: slab-use-after-free in mas_next_slot+0x1e95/0x2860
Read of size 8 at addr ffff8881160bdc00 by task pool-e/5770
CPU: 1 UID: 0 PID: 5770 Comm: pool-e Not tainted 7.2.0-rc7-1-debug+ #106
Allocated by task 5770:
  mas_alloc_nodes <- mas_preallocate <- __split_vma <- vms_gather_munmap_vmas
    <- do_vmi_align_munmap <- do_vmi_munmap <- __vm_munmap <- elf_load
    <- load_elf_binary <- bprm_execve          (pool-e's exec)
Freed by task 25 (ksoftirqd):
  __rcu_free_sheaf_prepare <- rcu_free_sheaf_nobarn <- rcu_do_batch <- rcu_core
The buggy address ... cache maple_node of size 256
freed 256-byte region [ffff8881160bdc00, ffff8881160bdd00)

There was also a more complex scenario when the node was immediately
recycled for the next split VMA insert, modified, but the access by
the iterator for the previous walk saw corrupted state and triggered
KASAN too.

Basically the problem is that any sleeping during VMA walks breaks the
RCU reader guarantees for the RCU Maple tree iterators. But sleeping
is unavoidable for various reasons.

I hit it with a kernel modification that makes this more likely
(It can do VMA splits on exec mm teardown)
and also in a very memory constrained environment (4GB guest running a
stress test), but based on code review I believe it's a generic problem that
could happen in a unmodified kernel.

That said I wasn't actually able to trigger it in a unmodified kernel
so far with stress testing.

The following old unsolved syzkaller report has a similar signature,
so maybe it was already seen:
https://syzkaller.appspot.com/bug?id=4c5268fbb1d6d508a4c34dc425e2693d1ff9911a

I guess in many cases where it happens for real you don't notice it
if you don't have KASAN active.

The patch fixes up all callers to maintain the RCU reader lock
regions correctly during the VMA walk. If they cannot be maintained the
iterator is refreshed by a new VMA address lookup in a new region, unless
it is proven safe not to.

In the cases where there is no sleeping it is strictly not needed
because this scenario could not happen due the existing VMA locking.

But I fixed them too to not violate the maple tree iterator
"rcu read lock or write lock" contract.

The ones that do not strictly need it are: count_mm_mlocked_page_nr,
remap_file_pages, range_contains_unmapped.

The ones that may sleep and clearly need it are: apply_vma_lock_flags,
do_mprotect_pkey, remap_move, mseal_apply, mbind_range, userfaultfd
register/unregister,
mwriteprotect_range (doesn't sleep in the walk itself, but uses iterator
after sleep)

In principle it could be optimized more, e.g. for example only do the
re-lookups when actual sleeping happened. Some of it could be done
with a new cond resched variant. But I tried to keep it minimal
for now.

The patch survived most of LTP, the kernel mm selftests and
my own stress tests. I didn't do any benchmarks.

For when it was introduced it's a complex area, but I believe the patch
that originally added the problem was
commit b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
Then
commit 17f1ae9b40c6 ("mm/vma: change munmap to use vma_munmap_struct() for
accounting and surrounding vmas")
extended the pattern to more places, and then later it was copied
elsewhere too. I'm mentioning only the first below.

No cc stable so far, needs some discussion first.

Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
Assisted-by: omp:gpt-5.6-luna
Signed-off-by: Andi Kleen <ak@kernel.org>
---
 mm/mempolicy.c   | 13 ++++++++
 mm/mlock.c       | 13 +++++++-
 mm/mmap.c        |  1 +
 mm/mprotect.c    | 29 +++++++++++++---
 mm/mremap.c      | 16 +++++++--
 mm/mseal.c       | 15 +++++++++
 mm/userfaultfd.c | 86 ++++++++++++++++++++++++++++++++++++++++++------
 mm/vma.c         | 13 +++++---
 8 files changed, 163 insertions(+), 23 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 3498a5651d50..7ffdb930b60d 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1062,7 +1062,20 @@ static int mbind_range(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
+	rcu_read_lock();
+	/*
+	 * The modify might have invalidated the iterators when
+	 * sleeping happened. Do another lookup
+	 */
+	vma_iter_set(vmi, vmstart);
+	vma = vma_find(vmi, vmend);
+	if (!vma) {
+		rcu_read_unlock();
+		return -ENOMEM;
+	}
 	*prev = vma;
+	rcu_read_unlock();
+	/* Iterator still protected by write lock */
 	return vma_replace_policy(vma, new_pol);
 }
 
diff --git a/mm/mlock.c b/mm/mlock.c
index efa6716e4dfb..ee5c0347129d 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -540,12 +540,15 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
 
 	nstart = start;
 	tmp = vma->vm_start;
+	rcu_read_lock();
 	for_each_vma_range(vmi, vma, end) {
 		int error;
 		vma_flags_t newflags;
 
-		if (vma->vm_start != tmp)
+		if (vma->vm_start != tmp) {
+			rcu_read_unlock();
 			return -ENOMEM;
+		}
 
 		newflags = vma->flags;
 		vma_flags_clear_mask(&newflags, VMA_LOCKED_MASK);
@@ -555,12 +558,19 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
 		tmp = vma->vm_end;
 		if (tmp > end)
 			tmp = end;
+		rcu_read_unlock();
 		error = mlock_fixup(&vmi, vma, &prev, nstart, tmp, &newflags);
 		if (error)
 			return error;
+		/*
+		 * The iterator is always left on a life node, so no
+		 * re-lookup needed after sleep.
+		 */
 		tmp = vma_iter_end(&vmi);
 		nstart = tmp;
+		rcu_read_lock();
 	}
+	rcu_read_unlock();
 
 	if (tmp < end)
 		return -ENOMEM;
@@ -589,6 +599,7 @@ static unsigned long count_mm_mlocked_page_nr(struct mm_struct *mm,
 	else
 		end = start + len;
 
+	guard(rcu)();
 	for_each_vma_range(vmi, vma, end) {
 		if (vma_test(vma, VMA_LOCKED_BIT)) {
 			if (start > vma->vm_start)
diff --git a/mm/mmap.c b/mm/mmap.c
index e10412160b32..dfe5c51dee98 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1184,6 +1184,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
 		VMA_ITERATOR(vmi, mm, vma->vm_end);
 		struct vm_area_struct *next, *prev = vma;
 
+		guard(rcu)();
 		for_each_vma_range(vmi, next, start + size) {
 			/* hole between vmas ? */
 			if (next->vm_start != prev->vm_end)
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 2888ee638d87..53e4fd45d39b 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -941,6 +941,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 	tlb_gather_mmu(&tlb, current->mm);
 	nstart = start;
 	tmp = vma->vm_start;
+	rcu_read_lock();
 	for_each_vma_range(vmi, vma, end) {
 		vm_flags_t mask_off_old_flags;
 		vma_flags_t new_vma_flags;
@@ -984,29 +985,49 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 			error = -EINVAL;
 			break;
 		}
+		rcu_read_unlock();
 
 		error = security_file_mprotect(vma, reqprot, prot);
-		if (error)
+		if (error) {
+			rcu_read_lock();
 			break;
-
+		}
+		rcu_read_lock();
+		/*
+		 * The security hook may sleep; re-lookup instead of
+		 * trusting the pre-sleep pointer.
+		 */
+		vma_iter_set(&vmi, vma->vm_start);
+		vma = vma_find(&vmi, end);
+		if (!vma) {
+			error = -ENOMEM;
+			break;
+		}
 		tmp = vma->vm_end;
 		if (tmp > end)
 			tmp = end;
+		rcu_read_unlock();
 
 		if (vma->vm_ops && vma->vm_ops->mprotect) {
 			error = vma->vm_ops->mprotect(vma, nstart, tmp, newflags);
-			if (error)
+			if (error) {
+				rcu_read_lock();
 				break;
+			}
 		}
 
 		error = mprotect_fixup(&vmi, &tlb, vma, &prev, nstart, tmp, newflags);
-		if (error)
+		if (error) {
+			rcu_read_lock();
 			break;
+		}
 
 		tmp = vma_iter_end(&vmi);
 		nstart = tmp;
 		prot = reqprot;
+		rcu_read_lock();
 	}
+	rcu_read_unlock();
 	tlb_finish_mmu(&tlb);
 
 	if (!error && tmp < end)
diff --git a/mm/mremap.c b/mm/mremap.c
index e8df5cdb0ac9..c8589c699515 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -1903,6 +1903,7 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
 	 * with all VMAs in the input range [addr, addr + old_len) being moved
 	 * (and split as necessary).
 	 */
+	rcu_read_lock();
 	for_each_vma_range(vmi, vma, end) {
 		/* Account for start, end not aligned with VMA start, end. */
 		unsigned long addr = max(vma->vm_start, start);
@@ -1911,8 +1912,10 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
 		bool multi_allowed;
 
 		/* No gap permitted at the start of the range. */
-		if (!seen_vma && start < vma->vm_start)
+		if (!seen_vma && start < vma->vm_start) {
+			rcu_read_unlock();
 			return -EFAULT;
+		}
 
 		/*
 		 * To sensibly move multiple VMAs, accounting for the fact that
@@ -1940,12 +1943,17 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
 		multi_allowed = vma_multi_allowed(vma);
 		if (!multi_allowed) {
 			/* This is not the first VMA, abort immediately. */
-			if (seen_vma)
+			if (seen_vma) {
+				rcu_read_unlock();
 				return -EFAULT;
+			}
 			/* This is the first, but there are more, abort. */
-			if (vma->vm_end < end)
+			if (vma->vm_end < end) {
+				rcu_read_unlock();
 				return -EFAULT;
+			}
 		}
+		rcu_read_unlock();
 
 		res_vma = check_prep_vma(vrm);
 		if (!res_vma)
@@ -1969,7 +1977,9 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
 		}
 		seen_vma = true;
 		target_addr = res_vma + vrm->new_len;
+		rcu_read_lock();
 	}
+	rcu_read_unlock();
 
 	return res;
 }
diff --git a/mm/mseal.c b/mm/mseal.c
index 7a8ac66dc215..551b5ad52f4d 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -22,6 +22,7 @@ static bool range_contains_unmapped(unsigned long start, unsigned long end)
 	unsigned long prev_end = start;
 	struct vm_area_struct *vma;
 
+	guard(rcu)();
 	for_each_vma_range(vmi, vma, end) {
 		if (vma->vm_start > prev_end)
 			return true;
@@ -43,6 +44,7 @@ static int __mseal_range(unsigned long start, unsigned long end)
 	if (start > vma->vm_start)
 		prev = vma;
 
+	rcu_read_lock();
 	for_each_vma_range(vmi, vma, end) {
 		const unsigned long curr_start = max(vma->vm_start, start);
 		const unsigned long curr_end = min(vma->vm_end, end);
@@ -51,17 +53,30 @@ static int __mseal_range(unsigned long start, unsigned long end)
 			vma_flags_t vma_flags = vma->flags;
 
 			vma_flags_set(&vma_flags, VMA_SEALED_BIT);
+			rcu_read_unlock();
 
 			vma = vma_modify_flags(&vmi, prev, vma, curr_start,
 					       curr_end, &vma_flags);
 			if (IS_ERR(vma))
 				return PTR_ERR(vma);
+
+			rcu_read_lock();
+			/* The modify may have slept and merged, so re-lookup. */
+			vma_iter_set(&vmi, curr_start);
+			vma = vma_find(&vmi, curr_end);
+			if (!vma) {
+				rcu_read_unlock();
+				return -ENOMEM;
+			}
+			rcu_read_unlock();
 			vma_start_write(vma);
 			vma_set_flags(vma, VMA_SEALED_BIT);
+			rcu_read_lock();
 		}
 
 		prev = vma;
 	}
+	rcu_read_unlock();
 
 	return 0;
 }
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 23fb68fce000..464570b15d4a 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1134,6 +1134,7 @@ static int mwriteprotect_range(struct userfaultfd_ctx *ctx, unsigned long start,
 		goto out_unlock;
 
 	err = -ENOENT;
+	rcu_read_lock();
 	for_each_vma_range(vmi, dst_vma, end) {
 
 		if (!userfaultfd_wp(dst_vma)) {
@@ -1150,14 +1151,18 @@ static int mwriteprotect_range(struct userfaultfd_ctx *ctx, unsigned long start,
 
 		_start = max(dst_vma->vm_start, start);
 		_end = min(dst_vma->vm_end, end);
+		rcu_read_unlock();
 
 		err = uffd_wp_range(dst_vma, _start, _end - _start, enable_wp);
+		rcu_read_lock();
+		/* The iterator is still on a life node */
 
 		/* Return 0 on success, <0 on failures */
 		if (err < 0)
 			break;
 		err = 0;
 	}
+	rcu_read_unlock();
 out_unlock:
 	up_read(&ctx->map_changing_lock);
 	mmap_read_unlock(dst_mm);
@@ -2324,9 +2329,17 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx,
 	if (vma->vm_start < start)
 		prev = vma;
 
+	rcu_read_lock();
 	for_each_vma_range(vmi, vma, end) {
+		rcu_read_unlock();
 		cond_resched();
 
+		rcu_read_lock();
+		/* The cond_resched above may have slept, so re-lookup. */
+		vma_iter_set(&vmi, vma->vm_start);
+		vma = vma_find(&vmi, end);
+		if (!vma)
+			break;
 		VM_WARN_ON_ONCE(!vma_can_userfault(vma, vm_flags, wp_async));
 		VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx &&
 				vma->vm_userfaultfd_ctx.ctx != ctx);
@@ -2355,6 +2368,7 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx,
 		new_vma_flags = vma->flags;
 		vma_flags_clear_mask(&new_vma_flags, __VMA_UFFD_FLAGS);
 		vma_flags_set_mask(&new_vma_flags, vma_flags);
+		rcu_read_unlock();
 
 		vma = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
 					    &new_vma_flags,
@@ -2368,15 +2382,31 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx,
 		 * the next vma was merged into the current one and
 		 * the current one has not been updated yet.
 		 */
+		rcu_read_lock();
+		/* The modify may have slept and merged, so re-lookup. */
+		vma_iter_set(&vmi, start);
+		vma = vma_find(&vmi, end);
+		if (!vma)
+			break;
+		rcu_read_unlock();
 		userfaultfd_set_ctx(vma, ctx, vm_flags);
+		rcu_read_lock();
 
-		if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
+		if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma)) {
+			rcu_read_unlock();
 			hugetlb_unshare_all_pmds(vma);
+			rcu_read_lock();
+			vma_iter_set(&vmi, start);
+			vma = vma_find(&vmi, end);
+			if (!vma)
+				break;
+		}
 
 skip:
 		prev = vma;
 		start = vma->vm_end;
 	}
+	rcu_read_unlock();
 
 	return 0;
 }
@@ -3808,16 +3838,26 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 	found = false;
 	basic_ioctls = false;
 	cur = vma;
+	rcu_read_lock();
 	do {
+		rcu_read_unlock();
 		cond_resched();
 
+		rcu_read_lock();
+		/* The cond_resched above may have slept, so re-lookup. */
+		vma_iter_set(&vmi, cur->vm_start);
+		cur = vma_find(&vmi, end);
+		if (!cur)
+			break;
 		VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
 				!!(cur->vm_flags & __VM_UFFD_FLAGS));
 
 		/* check not compatible vmas */
 		ret = -EINVAL;
-		if (!vma_can_userfault(cur, vm_flags, wp_async))
+		if (!vma_can_userfault(cur, vm_flags, wp_async)) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * RWP uses protnone as an access-tracking marker. PROT_NONE
@@ -3827,8 +3867,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 		 * mprotect() must still be unregisterable, so this is not
 		 * part of vma_can_userfault().
 		 */
-		if ((vm_flags & VM_UFFD_RWP) && !vma_is_accessible(cur))
+		if ((vm_flags & VM_UFFD_RWP) && !vma_is_accessible(cur)) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * UFFDIO_COPY will fill file holes even without
@@ -3839,8 +3881,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 		 * F_WRITE_SEAL can be taken until the vma is destroyed.
 		 */
 		ret = -EPERM;
-		if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
+		if (unlikely(!(cur->vm_flags & VM_MAYWRITE))) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * If this vma contains ending address, and huge pages
@@ -3852,11 +3896,15 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 
 			ret = -EINVAL;
 
-			if (end & (vma_hpagesize - 1))
+			if (end & (vma_hpagesize - 1)) {
+				rcu_read_unlock();
 				goto out_unlock;
+			}
 		}
-		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE))
+		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE)) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * Check that this vma isn't already owned by a
@@ -3866,8 +3914,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 		 */
 		ret = -EBUSY;
 		if (cur->vm_userfaultfd_ctx.ctx &&
-		    cur->vm_userfaultfd_ctx.ctx != ctx)
+		    cur->vm_userfaultfd_ctx.ctx != ctx) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * Mode switches that drop VM_UFFD_WP or VM_UFFD_RWP would
@@ -3876,8 +3926,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 		 * into the other mode. Require an unregister first.
 		 */
 		if (cur->vm_userfaultfd_ctx.ctx == ctx &&
-		    cur->vm_flags & (VM_UFFD_WP | VM_UFFD_RWP) & ~vm_flags)
+		    cur->vm_flags & (VM_UFFD_WP | VM_UFFD_RWP) & ~vm_flags) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * Note vmas containing huge pages
@@ -3887,6 +3939,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 
 		found = true;
 	} for_each_vma_range(vmi, cur, end);
+	rcu_read_unlock();
 	VM_WARN_ON_ONCE(!found);
 
 	ret = userfaultfd_register_range(ctx, vma, vm_flags, start, end,
@@ -3980,9 +4033,17 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 	 */
 	found = false;
 	cur = vma;
+	rcu_read_lock();
 	do {
+		rcu_read_unlock();
 		cond_resched();
 
+		rcu_read_lock();
+		/* The cond_resched above may have slept, so re-lookup. */
+		vma_iter_set(&vmi, cur->vm_start);
+		cur = vma_find(&vmi, end);
+		if (!cur)
+			break;
 		VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
 				!!(cur->vm_flags & __VM_UFFD_FLAGS));
 
@@ -3991,8 +4052,10 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 		 * the one used for registration.
 		 */
 		if (cur->vm_userfaultfd_ctx.ctx &&
-		    cur->vm_userfaultfd_ctx.ctx != ctx)
+		    cur->vm_userfaultfd_ctx.ctx != ctx) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		/*
 		 * Check not compatible vmas, not strictly required
@@ -4001,11 +4064,14 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 		 * provides for more strict behavior to notice
 		 * unregistration errors.
 		 */
-		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
+		if (!vma_can_userfault(cur, cur->vm_flags, wp_async)) {
+			rcu_read_unlock();
 			goto out_unlock;
+		}
 
 		found = true;
 	} for_each_vma_range(vmi, cur, end);
+	rcu_read_unlock();
 	VM_WARN_ON_ONCE(!found);
 
 	vma_iter_set(&vmi, start);
diff --git a/mm/vma.c b/mm/vma.c
index 35e7a64855fa..e00b0cdc3d83 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -614,10 +614,8 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	validate_mm(vma->vm_mm);
 
 	/* Success. */
-	if (new_below)
-		vma_next(vmi);
-	else
-		vma_prev(vmi);
+	vma_iter_set(vmi, vma->vm_start);
+	vma_find(vmi, ULONG_MAX);
 
 	return 0;
 
@@ -1573,7 +1571,12 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
 #endif
 	}
 
-	vms->next = vma_next(vms->vmi);
+	/*
+	 * The loop's cached node may be the one a split's store deferred.
+	 * Continue from the range end.
+	 */
+	vma_iter_set(vms->vmi, vms->end);
+	vms->next = vma_find(vms->vmi, ULONG_MAX);
 	if (vms->next)
 		vms->unmap_end = vms->next->vm_start;
 
-- 
2.54.0

Re: [PATCH] RCU safety for vma maple tree walks

[Pedro Falcato]

On Mon, Aug 31, 2026 at 07:35:11AM -0700, Andi Kleen wrote:
> I ran into the following scenario in a slightly modified kernel:
> 
>  1. vms_gather_munmap_vmas walks the unmap range and caches a maple node N
>     in the maple iterator.
>  2. It triggers a __split_vma to fix up a range and during that node N
>     is queued for freeing with kfree_rcu
>  3. There is another __split_vma that allocates memory and sleeps due to
>     memory pressure.
>  4. During the sleep the grace period expires and node N gets freed for
>     real.
>  5. The iterator still has node N cached
>  6. When the iteration continues it accesses the freed node and KASAN
>     trips:
> 
> BUG: KASAN: slab-use-after-free in mas_next_slot+0x1e95/0x2860
> Read of size 8 at addr ffff8881160bdc00 by task pool-e/5770
> CPU: 1 UID: 0 PID: 5770 Comm: pool-e Not tainted 7.2.0-rc7-1-debug+ #106
> Allocated by task 5770:
>   mas_alloc_nodes <- mas_preallocate <- __split_vma <- vms_gather_munmap_vmas
>     <- do_vmi_align_munmap <- do_vmi_munmap <- __vm_munmap <- elf_load
>     <- load_elf_binary <- bprm_execve          (pool-e's exec)
> Freed by task 25 (ksoftirqd):
>   __rcu_free_sheaf_prepare <- rcu_free_sheaf_nobarn <- rcu_do_batch <- rcu_core
> The buggy address ... cache maple_node of size 256
> freed 256-byte region [ffff8881160bdc00, ffff8881160bdd00)
> 
> There was also a more complex scenario when the node was immediately
> recycled for the next split VMA insert, modified, but the access by
> the iterator for the previous walk saw corrupted state and triggered
> KASAN too.
> 
> Basically the problem is that any sleeping during VMA walks breaks the
> RCU reader guarantees for the RCU Maple tree iterators. But sleeping
> is unavoidable for various reasons.
> 
> I hit it with a kernel modification that makes this more likely
> (It can do VMA splits on exec mm teardown)
> and also in a very memory constrained environment (4GB guest running a
> stress test), but based on code review I believe it's a generic problem that
> could happen in a unmodified kernel.
> 
> That said I wasn't actually able to trigger it in a unmodified kernel
> so far with stress testing.
> 
> The following old unsolved syzkaller report has a similar signature,
> so maybe it was already seen:
> https://syzkaller.appspot.com/bug?id=4c5268fbb1d6d508a4c34dc425e2693d1ff9911a
> 
> I guess in many cases where it happens for real you don't notice it
> if you don't have KASAN active.
> 
> The patch fixes up all callers to maintain the RCU reader lock
> regions correctly during the VMA walk. If they cannot be maintained the
> iterator is refreshed by a new VMA address lookup in a new region, unless
> it is proven safe not to.
> 

But none of this code uses RCU? I'm confused. The maple tree state should not
be keeping bad state. That is a bug.

All of these functions take the mmap write lock. That should exclude
against other concurrent changes. Using RCU here makes no logical sense.

Does the kernel say anything interesting when CONFIG_DEBUG_VM_MAPLE_TREE=y?
 

-- 
Pedro

Re: [PATCH] RCU safety for vma maple tree walks

[Andi Kleen]

> But none of this code uses RCU? I'm confused. 

There are two classes of callers: ones that change the maple tree while
walking and those that don't. The ones that change it use 
kfree_rcu (if that's what you meant with "use RCU") because they can free
nodes.

The patch changes both classes, but strictly only the ones actually
changing need the change. For the others the existing locking is
indeed sufficient.


> The maple tree state should not
> be keeping bad state. That is a bug.

It's more the iterators caching bad nodes, not the maple tree itself.

> 
> All of these functions take the mmap write lock. That should exclude
> against other concurrent changes. Using RCU here makes no logical sense.

There's no concurrent changes, it's all local splits.

> 
> Does the kernel say anything interesting when CONFIG_DEBUG_VM_MAPLE_TREE=y?

Was indeed missing. I'll try that.

-Andi

Re: [PATCH] RCU safety for vma maple tree walks

[Lorenzo Stoakes (ARM)]

On Mon, Aug 31, 2026 at 12:35:30PM -0700, Andi Kleen wrote:
> > But none of this code uses RCU? I'm confused.
>
> There are two classes of callers: ones that change the maple tree while
> walking and those that don't. The ones that change it use
> kfree_rcu (if that's what you meant with "use RCU") because they can free
> nodes.
>
> The patch changes both classes, but strictly only the ones actually
> changing need the change. For the others the existing locking is
> indeed sufficient.

Andi I think you're really wildly off the mark here and it's not really a great
use of our time to help you debug your own private patch.

But in case you are on to a real bug here and I'm wildly wrong, it
shouldn't be difficult for you to point out precisely where the node is being
freed as a result of the split that causes a stale node to be referenced?

I mean what actually modifies the maple tree nodes here?

__split_vma()
  -> vma_complete()
    -> vma_iter_store_new()
      -> vma_iter_store_overwrite()

Right?

But I see:

	if (vmi->mas.status != ma_start &&
	    ((vmi->mas.index > vma->vm_start) || (vmi->mas.last < vma->vm_start)))
		vma_iter_invalidate(vmi);

Which calls mas_pause() which sets mas->node = NULL.

So I mean, presumably you are saying this doesn't work correctly or this
criteria is wrong, I can't really see how else there could be a problem here,
could you explain exactly what's up here?

Also why is the solution to insert a whole bunch of RCU read locks everywhere so
we can keep on accessing a node that we've already decided to free?

I mean surely the solution really ought to be simply invalidating the iterator
right?

Also again, could you share the patch you've applied to the kernel you're
actually seeing this bug in, given you haven't reproduced it even once with an
upstream kernel?

Thanks.

--
Cheers, Lorenzo

Re: [PATCH] RCU safety for vma maple tree walks

[Andi Kleen]

On Mon, Aug 31, 2026 at 08:55:37PM +0100, Lorenzo Stoakes (ARM) wrote:
> I mean what actually modifies the maple tree nodes here?
> 
> __split_vma()
>   -> vma_complete()
>     -> vma_iter_store_new()
>       -> vma_iter_store_overwrite()
> 
> Right?

The crash was on the walk, but yes the modification likely causes it.

> 
> But I see:
> 
> 	if (vmi->mas.status != ma_start &&
> 	    ((vmi->mas.index > vma->vm_start) || (vmi->mas.last < vma->vm_start)))
> 		vma_iter_invalidate(vmi);
> 
> Which calls mas_pause() which sets mas->node = NULL.
> 
> So I mean, presumably you are saying this doesn't work correctly or this
> criteria is wrong, I can't really see how else there could be a problem here,
> could you explain exactly what's up here?

I can't explain it currently, but yes something wrong with that logic
is a good theory.

I have a (somewhat garbled) processor trace log of the failure, but
it doesn't quite have enough information to untangle it completely.

The original bug also happened in a very memory constrained environment
(4GB guest), but it's somewhat hard to reproduce it in a setup
that still has enough memory to do useful debugging.

I'm currently working on the reproducer on the vanilla kernel.

So far I made some progress to get something that looks closer to the
original trace, but I need artificial sleeps at the vma allocation point
and it still didn't fully reproduce the original scenario yet. 

There was actually one crash with a different
signature, but I haven't analyzed it so far [1].

The WIP stresser that forces something that is close to the 
uprobes is here: 
https://firstfloor.org/~andi/madvise-dontfork-stress.c

> Also why is the solution to insert a whole bunch of RCU read locks everywhere so
> we can keep on accessing a node that we've already decided to free?

I may have an old school understanding of RCU, but I was always
thinking that the rcu read sections are needed for any readers with
preemption. The walker is clearly a reader.

But I guess in this particular case it's not true because
the writer lock and the invalidation is enough. It still seems a little
dubious with all the preemption cases, but at least I cannot see a clear
hole.

> 
> I mean surely the solution really ought to be simply invalidating the iterator
> right?

Yes I guess. It would certainly be simpler.


> 
> Also again, could you share the patch you've applied to the kernel you're
> actually seeing this bug in, given you haven't reproduced it even once with an
> upstream kernel?

Sure it's this patchkit on l-k that adds some new functionality to
uprobes: https://lore.kernel.org/lkml/20260831150651.1134594-1-ak@kernel.org/
(or for more information
https://lore.kernel.org/lkml/20260831150651.1134594-16-ak@kernel.org/ )

I don't think it actually changes any core VM locking or really how the uprobes
interact with the VM code, but it changes timing and makes it easier
to have some obscure DONTCOPY VMA setups. The actual VM code is not
changed.

The code that changes the core uprobes behavior is 5/19, but it can't really
be fully exercised without some of the later patches.

Also the posted version has some issues (at least Sashiko pointed out 
some real problems), but I don't believe it affects this.

-Andi


[1]


[   66.689807][  T532] BUG: unable to handle page fault for address: ffffebde00477288
[   66.691116][  T532] #PF: supervisor read access in kernel mode
[   66.692227][  T532] #PF: error_code(0x0000) - not-present page
[   66.693267][  T532] PGD 0 P4D 0 
[   66.693841][  T532] Oops: Oops: 0000 [#1] SMP KASAN NOPTI
[   66.694596][  T532] CPU: 1 UID: 0 PID: 532 Comm: madvise-dontfor Not tainted 7.2.0-1-debug+ #2 PREEMPT(full)  967bbfd33d6729a809cf8db6188ec03a118152c4
[   66.697960][  T532] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014
[   66.700523][  T532] RIP: 0010:qlist_free_all+0x93/0x130
[   66.701296][  T532] Code: c2 4c 01 f2 0f 82 a3 00 00 00 48 c7 c1 00 00 00 80 48 2b 0d b7 c9 34 03 48 01 ca 48 c1 ea 0c 48 c1 e2 06 48 03 15 95 c9 34 03 <48> 8b 4a 08 48 89 ce 83 e6 01 48 83 ee 01 48 09 f1 48 21 ca 31 c9
[   66.702949][  T532] RSP: 0018:ffff8881097a7478 EFLAGS: 00010282
[   66.703474][  T532] RAX: 0000000011dca450 RBX: 0000000000000000 RCX: 0000777f80000000
[   66.704728][  T532] RDX: ffffebde00477280 RSI: ffffea000447ad40 RDI: 0000000000200000
[   66.706497][  T532] RBP: 0000000011dca450 R08: 0000000000000001 R09: ffffffff9a58ccfe
[   66.708160][  T532] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
[   66.709799][  T532] R13: ffff8881097a74b0 R14: 0000000080000000 R15: ffff888111eb5d00
[   66.711393][  T532] FS:  00007f2990b12780(0000) GS:ffff888228ea7000(0000) knlGS:0000000000000000
[   66.713158][  T532] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   66.714486][  T532] CR2: ffffebde00477288 CR3: 0000000103565005 CR4: 0000000000f70ef0
[   66.715595][  T532] PKRU: 55555554
[   66.715972][  T532] Call Trace:
[   66.716320][  T532]  <TASK>
[   66.716629][  T532]  kasan_quarantine_reduce+0x19a/0x250
[   66.717190][  T532]  __kasan_slab_alloc+0x6a/0x90
[   66.717706][  T532]  kmem_cache_alloc_noprof+0x214/0x6b0
[   66.718285][  T532]  ? vm_area_dup+0x2b/0x970
[   66.718763][  T532]  vm_area_dup+0x2b/0x970
[   66.719211][  T532]  ? msleep+0x1b/0x30
[   66.719625][  T532]  __split_vma+0x32e/0xbb0
[   66.720086][  T532]  ? __pfx___split_vma+0x10/0x10
[   66.720596][  T532]  ? __pfx_mas_prev+0x10/0x10
[   66.721095][  T532]  ? lock_is_held_type+0xfa/0x1c0
[   66.721613][  T532]  vma_modify+0x1a50/0x24e0
[   66.722087][  T532]  ? __pfx_vma_modify+0x10/0x10
[   66.722593][  T532]  ? lock_is_held_type+0xfa/0x1c0
[   66.723110][  T532]  vma_modify_flags+0x2ed/0x4e0
[   66.723594][  T532]  ? __pfx_vma_modify_flags+0x10/0x10
[   66.724144][  T532]  ? mas_prev_slot+0x328/0x1d30
[   66.724655][  T532]  mprotect_fixup+0x226/0xb90
[   66.725142][  T532]  ? __pfx_mprotect_fixup+0x10/0x10
[   66.725696][  T532]  ? mas_prev_slot+0x328/0x1d30
[   66.726195][  T532]  ? lock_is_held_type+0xfa/0x1c0
[   66.726706][  T532]  ? mas_next_slot+0xa7a/0x20a0
[   66.727200][  T532]  ? lock_sequence+0xd7/0x180
[   66.727676][  T532]  do_mprotect_pkey+0x81d/0xb40
[   66.728167][  T532]  ? __pfx_do_mprotect_pkey+0x10/0x10
[   66.728702][  T532]  ? rcu_is_watching+0x16/0xb0
[   66.729193][  T532]  ? do_syscall_64+0x203/0x6a0
[   66.729696][  T532]  ? lockdep_hardirqs_on+0x95/0x140
[   66.730226][  T532]  ? entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   66.730811][  T532]  ? do_vmi_munmap+0x159/0x2e0
[   66.731226][  T532]  ? __vm_munmap+0x1dc/0x360
[   66.731635][  T532]  ? __do_sys_mincore+0x4b6/0x6c0
[   66.732129][  T532]  __x64_sys_mprotect+0x78/0xe0
[   66.732616][  T532]  ? lockdep_hardirqs_on+0x95/0x140
[   66.733140][  T532]  ? do_syscall_64+0x83/0x6a0
[   66.733611][  T532]  do_syscall_64+0xf6/0x6a0
[   66.734068][  T532]  ? trace_hardirqs_on_prepare+0x13d/0x190
[   66.734650][  T532]  ? lockdep_hardirqs_on+0x95/0x140
[   66.735185][  T532]  ? entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   66.735786][  T532]  ? do_syscall_64+0x221/0x6a0
[   66.736264][  T532]  ? rcu_is_watching+0x16/0xb0
[   66.736746][  T532]  ? rcu_is_watching+0x16/0xb0
[   66.737233][  T532]  ? do_syscall_64+0x203/0x6a0
[   66.737721][  T532]  ? trace_hardirqs_on_prepare+0x13d/0x190
[   66.738302][  T532]  ? lockdep_hardirqs_on+0x95/0x140
[   66.738820][  T532]  ? entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   66.739924][  T532]  ? do_syscall_64+0x221/0x6a0
[   66.740457][  T532]  ? do_syscall_64+0x203/0x6a0
[   66.740934][  T532]  ? trace_hardirqs_on_prepare+0x13d/0x190
[   66.741518][  T532]  ? lockdep_hardirqs_on+0x95/0x140
[   66.742038][  T532]  ? entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   66.742638][  T532]  ? rcu_is_watching+0x16/0xb0
[   66.743115][  T532]  ? do_syscall_64+0x31/0x6a0
[   66.743582][  T532]  ? trace_hardirqs_off_finish+0x13d/0x190
[   66.744172][  T532]  ? lockdep_hardirqs_off+0xb3/0x100
[   66.744717][  T532]  ? do_syscall_64+0x64/0x6a0
[   66.745212][  T532]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   66.745834][  T532] RIP: 0033:0x7f2990c22667
[   66.746301][  T532] Code: ef e8 dd f9 ff ff 84 c0 75 b9 31 db 48 83 c4 08 48 89 d8 5b 5d 41 5c 41 5d 41 5e 41 5f c3 0f 1f 44 00 00 b8 0a 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 79 77 0d 00 f7 d8 64 89 01 48
[   66.748156][  T532] RSP: 002b:00007fffb8fba1e8 EFLAGS: 00000202 ORIG_RAX: 000000000000000a
[   66.748885][  T532] RAX: ffffffffffffffda RBX: 0000000000004000 RCX: 00007f2990c22667
[   66.749575][  T532] RDX: 0000000000000001 RSI: 0000000000004000 RDI: 00007f2990af6000
[   66.750260][  T532] RBP: 0000000000000008 R08: 00000000ffffffff R09: 0000000000000000
[   66.750942][  T532] R10: 0000000000000022 R11: 0000000000000202 R12: 00007f2990af2000
[   66.751948][  T532] R13: 0000000000020000 R14: 00007f2990af6000 R15: 0000000000000001
[   66.752777][  T532]  </TASK>
[   66.753093][  T532] Modules linked in:
[   66.753516][  T532] CR2: ffffebde00477288
[   66.753935][  T532] ---[ end trace 0000000000000000 ]---
[   66.754496][  T532] RIP: 0010:qlist_free_all+0x93/0x130
[   66.754503][  T532] Code: c2 4c 01 f2 0f 82 a3 00 00 00 48 c7 c1 00 00 00 80 48 2b 0d b7 c9 34 03 48 01 ca 48 c1 ea 0c 48 c1 e2 06 48 03 15 95 c9 34 03 <48> 8b 4a 08 48 89 ce 83 e6 01 48 83 ee 01 48 09 f1 48 21 ca 31 c9
[   66.754505][  T532] RSP: 0018:ffff8881097a7478 EFLAGS: 00010282
[   66.754508][  T532] RAX: 0000000011dca450 RBX: 0000000000000000 RCX: 0000777f80000000
[   66.754509][  T532] RDX: ffffebde00477280 RSI: ffffea000447ad40 RDI: 0000000000200000
[   66.754511][  T532] RBP: 0000000011dca450 R08: 0000000000000001 R09: ffffffff9a58ccfe
[   66.754512][  T532] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
[   66.754513][  T532] R13: ffff8881097a74b0 R14: 0000000080000000 R15: ffff888111eb5d00
[   66.754515][  T532] FS:  00007f2990b12780(0000) GS:ffff888228ea7000(0000) knlGS:0000000000000000
[   66.754517][  T532] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   66.754518][  T532] CR2: ffffebde00477288 CR3: 0000000103565005 CR4: 0000000000f70ef0
[   66.754524][  T532] PKRU: 55555554
[   66.754526][  T532] note: madvise-dontfor[532] exited with irqs disabled

Re: [PATCH] RCU safety for vma maple tree walks

[Lorenzo Stoakes (ARM)]

Hi Andi,

Looking at https://lore.kernel.org/lkml/20260831150651.1134594-6-ak@kernel.org/
I can see the cause of your issue - it's your own patch, exactly as we've been
telling you.

You're inserting a VMA on a uprobe_mmap() call, which is completely incorrect:

<munmap()>
  -> vma_complete()
    -> uprobe_mmap(vp->insert)
      -> install_breakpoint()
        -> ...
	  -> create_uprobe_ptwrite_page()
	    -> _install_special_mapping()

Now you're inserting a VMA, using an entirely distinct iterator, while an
existing VMA iterator is in use.

That means you can end up rebalancing the tree and _trashing the iterator_ in
the core mm code.

Your (LLM's) proposed solution to this was to keep on doing the completely
broken thing, but by inserting various RCU locks, preventing a KASAN report on
use-after-free's due to avoiding an RCU grace period expiring.

You can't reproduce this upstream, because the bug is in your (RFC) series.

So, exactly as we've been trying to tell you.

> Assisted-by: omp:gpt-5.6-luna

I'm a little grumpy about this as you've taken up a bunch of our time to assess
two sets of completely broken LLM-generated code, and you've mostly responded by
denying what the authors of this code have repeatedly told you.

You also seem to be using one of the lightest weight LLM models - it isn't a
great choice for core kernel work, especially in areas you are unfamiliar with.

In future, please audit the code and make sure you understand every part of it
as per https://docs.kernel.org/process/generated-content.html

	As with the output of any tooling, the result may be incorrect or
	inappropriate. You are expected to understand and to be able to defend
	everything you submit. If you are unable to do so, then do not submit
	the resulting changes.

Please respect our time moving forwards, thanks.

--
Cheers, Lorenzo

Re: [PATCH] RCU safety for vma maple tree walks

[Lorenzo Stoakes (ARM)]

you really should have sent this as an RFC or a discussion.

On Mon, Aug 31, 2026 at 07:35:11AM -0700, Andi Kleen wrote:
> I ran into the following scenario in a slightly modified kernel:

I mean I could stop here :) you are reporting a bug for a kernel that isn't
the one upstream.

You say your change "can do VMA splits on exec mm teardown" - that sounds
very much like the root of the problem :)

>
>  1. vms_gather_munmap_vmas walks the unmap range and caches a maple node N
>     in the maple iterator.
>  2. It triggers a __split_vma to fix up a range and during that node N
>     is queued for freeing with kfree_rcu

This makes absolutely no sense, we are not allocating maple nodes then
immediately freeing them while also retaining them in the same iterator.

The mmap write lock serialises writes to the maple tree also so only the
current task can do this except... if the kernel is patched :)

If it's a store that invalidates the node, then that same operation also
moves the same iterator forawrd.

>  3. There is another __split_vma that allocates memory and sleeps due to
>     memory pressure.
>  4. During the sleep the grace period expires and node N gets freed for
>     real.

But... what exactly caused the node to be deleted and the iterator to
somehow remain valid?

>  5. The iterator still has node N cached
>  6. When the iteration continues it accesses the freed node and KASAN
>     trips:

What you're describing simply isn't possible.

The mm maple tree is locked under MT_FLAGS_LOCK_EXTERN |
MT_FLAGS_USE_RCU. We use the VMA/mmap locks to serialise.

The mmap write lock serialises the only possible writer to the current
task.

If there was some way of doing this we 100% would have hit this
before. Hell, syzkaller and friends have hit the most outlandish stuff
imaginable.

In the patch you describe the only stores are being done by the called
__split_vma() calls which... use the same iterator?

As you can see from the sig:

static __must_check int
__split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
	    unsigned long addr, int new_below)

So I think what's happening is that your patch that "can do VMA splits on
exec mm teardown" is doing something wrong here.

>
> BUG: KASAN: slab-use-after-free in mas_next_slot+0x1e95/0x2860
> Read of size 8 at addr ffff8881160bdc00 by task pool-e/5770
> CPU: 1 UID: 0 PID: 5770 Comm: pool-e Not tainted 7.2.0-rc7-1-debug+ #106
> Allocated by task 5770:
>   mas_alloc_nodes <- mas_preallocate <- __split_vma <- vms_gather_munmap_vmas
>     <- do_vmi_align_munmap <- do_vmi_munmap <- __vm_munmap <- elf_load
>     <- load_elf_binary <- bprm_execve          (pool-e's exec)
> Freed by task 25 (ksoftirqd):
>   __rcu_free_sheaf_prepare <- rcu_free_sheaf_nobarn <- rcu_do_batch <- rcu_core
> The buggy address ... cache maple_node of size 256
> freed 256-byte region [ffff8881160bdc00, ffff8881160bdd00)

This makes no sense, except... if something wrongly starts writing into
mm_mt without proper locking, like... a custom patch in the kernel :)

>
> There was also a more complex scenario when the node was immediately
> recycled for the next split VMA insert, modified, but the access by
> the iterator for the previous walk saw corrupted state and triggered
> KASAN too.
>
> Basically the problem is that any sleeping during VMA walks breaks the
> RCU reader guarantees for the RCU Maple tree iterators. But sleeping

What RCU reader guarantees?

> is unavoidable for various reasons.
>
> I hit it with a kernel modification that makes this more likely
> (It can do VMA splits on exec mm teardown)

Smoking gun...

> and also in a very memory constrained environment (4GB guest running a
> stress test), but based on code review I believe it's a generic problem that
> could happen in a unmodified kernel.
>
> That said I wasn't actually able to trigger it in a unmodified kernel
> so far with stress testing.

Yes because the bug almost certainly doesn't exist in mainline :)

>
> The following old unsolved syzkaller report has a similar signature,
> so maybe it was already seen:
> https://syzkaller.appspot.com/bug?id=4c5268fbb1d6d508a4c34dc425e2693d1ff9911a

Hmm that is a very old report? That could have any number of causes?

>
> I guess in many cases where it happens for real you don't notice it
> if you don't have KASAN active.
>
> The patch fixes up all callers to maintain the RCU reader lock
> regions correctly during the VMA walk. If they cannot be maintained the
> iterator is refreshed by a new VMA address lookup in a new region, unless
> it is proven safe not to.
>
> In the cases where there is no sleeping it is strictly not needed
> because this scenario could not happen due the existing VMA locking.
>
> But I fixed them too to not violate the maple tree iterator
> "rcu read lock or write lock" contract.

I'm not sure that's a contract that exists? RCU lock or external lock
yes...

>
> The ones that do not strictly need it are: count_mm_mlocked_page_nr,
> remap_file_pages, range_contains_unmapped.
>
> The ones that may sleep and clearly need it are: apply_vma_lock_flags,
> do_mprotect_pkey, remap_move, mseal_apply, mbind_range, userfaultfd
> register/unregister,
> mwriteprotect_range (doesn't sleep in the walk itself, but uses iterator
> after sleep)
>
> In principle it could be optimized more, e.g. for example only do the
> re-lookups when actual sleeping happened. Some of it could be done
> with a new cond resched variant. But I tried to keep it minimal
> for now.
>
> The patch survived most of LTP, the kernel mm selftests and
> my own stress tests. I didn't do any benchmarks.
>
> For when it was introduced it's a complex area, but I believe the patch
> that originally added the problem was
> commit b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
> Then
> commit 17f1ae9b40c6 ("mm/vma: change munmap to use vma_munmap_struct() for
> accounting and surrounding vmas")
> extended the pattern to more places, and then later it was copied
> elsewhere too. I'm mentioning only the first below.

This seems like absolute handwaving, sorry.

>
> No cc stable so far, needs some discussion first.

I mean RFC is for that right? :)

>
> Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
> Assisted-by: omp:gpt-5.6-luna

Yup, it shows. I think this isn't a great showcase for gpt...

I have to say this isn't the way to engage on this stuff Andi - you're
essentially workslopping us when you really you should have _opened_ with a
discussion and copied the actual patch you've made to the kernel.

> Signed-off-by: Andi Kleen <ak@kernel.org>

This patch is wrong even if your theory is right, and the patch is utterly
horrible from a maintenance point of view anyway.

> ---
>  mm/mempolicy.c   | 13 ++++++++
>  mm/mlock.c       | 13 +++++++-
>  mm/mmap.c        |  1 +
>  mm/mprotect.c    | 29 +++++++++++++---
>  mm/mremap.c      | 16 +++++++--
>  mm/mseal.c       | 15 +++++++++
>  mm/userfaultfd.c | 86 ++++++++++++++++++++++++++++++++++++++++++------
>  mm/vma.c         | 13 +++++---
>  8 files changed, 163 insertions(+), 23 deletions(-)

The changes are very horrible, even if we did need to do something like this, it
really couldn't be in this form.

This introduces assumptions about not sleeping all over, it breaks the
abstraction, I mean it's pretty patently obviously not upstreamable.

But you also do weird stuff like... assuming an rcu_read_lock() after a
sleep somehow restores the node back from death...

This could all have been cleared up and saved people time had you just
kicked off a discussion thread or pinged us.

>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 3498a5651d50..7ffdb930b60d 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1062,7 +1062,20 @@ static int mbind_range(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	if (IS_ERR(vma))
>  		return PTR_ERR(vma);
>
> +	rcu_read_lock();
> +	/*
> +	 * The modify might have invalidated the iterators when
> +	 * sleeping happened. Do another lookup
> +	 */
> +	vma_iter_set(vmi, vmstart);
> +	vma = vma_find(vmi, vmend);
> +	if (!vma) {
> +		rcu_read_unlock();
> +		return -ENOMEM;
> +	}
>  	*prev = vma;
> +	rcu_read_unlock();

If your RCU theory were true, then the iterator now has invalid nodes in it
then when next you RCU lock -> walk you are going to UAF?

You're also forcing a complete rewalk every single time and making the vmi
pointless here.

> +	/* Iterator still protected by write lock */
>  	return vma_replace_policy(vma, new_pol);
>  }
>
> diff --git a/mm/mlock.c b/mm/mlock.c
> index efa6716e4dfb..ee5c0347129d 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -540,12 +540,15 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>
>  	nstart = start;
>  	tmp = vma->vm_start;
> +	rcu_read_lock();
>  	for_each_vma_range(vmi, vma, end) {
>  		int error;
>  		vma_flags_t newflags;
>
> -		if (vma->vm_start != tmp)
> +		if (vma->vm_start != tmp) {
> +			rcu_read_unlock();
>  			return -ENOMEM;
> +		}
>
>  		newflags = vma->flags;
>  		vma_flags_clear_mask(&newflags, VMA_LOCKED_MASK);
> @@ -555,12 +558,19 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>  		tmp = vma->vm_end;
>  		if (tmp > end)
>  			tmp = end;
> +		rcu_read_unlock();
>  		error = mlock_fixup(&vmi, vma, &prev, nstart, tmp, &newflags);
>  		if (error)
>  			return error;
> +		/*
> +		 * The iterator is always left on a life node, so no

live?

> +		 * re-lookup needed after sleep.

This makes no sense, your theory is that an RCU grace period can invalidate
any node right, somehow?

> +		 */
>  		tmp = vma_iter_end(&vmi);
>  		nstart = tmp;
> +		rcu_read_lock();
>  	}
> +	rcu_read_unlock();

Now nested rcu locks, to add to complexity...

I mean I could go on but I think the LLM has got itself very confused here
and a human should have intervened sooner.

>
>  	if (tmp < end)
>  		return -ENOMEM;
> @@ -589,6 +599,7 @@ static unsigned long count_mm_mlocked_page_nr(struct mm_struct *mm,
>  	else
>  		end = start + len;
>
> +	guard(rcu)();
>  	for_each_vma_range(vmi, vma, end) {
>  		if (vma_test(vma, VMA_LOCKED_BIT)) {
>  			if (start > vma->vm_start)
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e10412160b32..dfe5c51dee98 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1184,6 +1184,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
>  		VMA_ITERATOR(vmi, mm, vma->vm_end);
>  		struct vm_area_struct *next, *prev = vma;
>
> +		guard(rcu)();
>  		for_each_vma_range(vmi, next, start + size) {
>  			/* hole between vmas ? */
>  			if (next->vm_start != prev->vm_end)
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 2888ee638d87..53e4fd45d39b 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -941,6 +941,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
>  	tlb_gather_mmu(&tlb, current->mm);
>  	nstart = start;
>  	tmp = vma->vm_start;
> +	rcu_read_lock();
>  	for_each_vma_range(vmi, vma, end) {
>  		vm_flags_t mask_off_old_flags;
>  		vma_flags_t new_vma_flags;
> @@ -984,29 +985,49 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
>  			error = -EINVAL;
>  			break;
>  		}
> +		rcu_read_unlock();
>
>  		error = security_file_mprotect(vma, reqprot, prot);
> -		if (error)
> +		if (error) {
> +			rcu_read_lock();
>  			break;
> -
> +		}
> +		rcu_read_lock();
> +		/*
> +		 * The security hook may sleep; re-lookup instead of
> +		 * trusting the pre-sleep pointer.
> +		 */
> +		vma_iter_set(&vmi, vma->vm_start);
> +		vma = vma_find(&vmi, end);
> +		if (!vma) {
> +			error = -ENOMEM;
> +			break;
> +		}
>  		tmp = vma->vm_end;
>  		if (tmp > end)
>  			tmp = end;
> +		rcu_read_unlock();
>
>  		if (vma->vm_ops && vma->vm_ops->mprotect) {
>  			error = vma->vm_ops->mprotect(vma, nstart, tmp, newflags);
> -			if (error)
> +			if (error) {
> +				rcu_read_lock();
>  				break;
> +			}
>  		}
>
>  		error = mprotect_fixup(&vmi, &tlb, vma, &prev, nstart, tmp, newflags);
> -		if (error)
> +		if (error) {
> +			rcu_read_lock();
>  			break;
> +		}
>
>  		tmp = vma_iter_end(&vmi);
>  		nstart = tmp;
>  		prot = reqprot;
> +		rcu_read_lock();
>  	}
> +	rcu_read_unlock();
>  	tlb_finish_mmu(&tlb);
>
>  	if (!error && tmp < end)
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e8df5cdb0ac9..c8589c699515 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1903,6 +1903,7 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  	 * with all VMAs in the input range [addr, addr + old_len) being moved
>  	 * (and split as necessary).
>  	 */
> +	rcu_read_lock();
>  	for_each_vma_range(vmi, vma, end) {
>  		/* Account for start, end not aligned with VMA start, end. */
>  		unsigned long addr = max(vma->vm_start, start);
> @@ -1911,8 +1912,10 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  		bool multi_allowed;
>
>  		/* No gap permitted at the start of the range. */
> -		if (!seen_vma && start < vma->vm_start)
> +		if (!seen_vma && start < vma->vm_start) {
> +			rcu_read_unlock();
>  			return -EFAULT;
> +		}
>
>  		/*
>  		 * To sensibly move multiple VMAs, accounting for the fact that
> @@ -1940,12 +1943,17 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  		multi_allowed = vma_multi_allowed(vma);
>  		if (!multi_allowed) {
>  			/* This is not the first VMA, abort immediately. */
> -			if (seen_vma)
> +			if (seen_vma) {
> +				rcu_read_unlock();
>  				return -EFAULT;
> +			}
>  			/* This is the first, but there are more, abort. */
> -			if (vma->vm_end < end)
> +			if (vma->vm_end < end) {
> +				rcu_read_unlock();
>  				return -EFAULT;
> +			}
>  		}
> +		rcu_read_unlock();
>
>  		res_vma = check_prep_vma(vrm);
>  		if (!res_vma)
> @@ -1969,7 +1977,9 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  		}
>  		seen_vma = true;
>  		target_addr = res_vma + vrm->new_len;
> +		rcu_read_lock();
>  	}
> +	rcu_read_unlock();
>
>  	return res;
>  }
> diff --git a/mm/mseal.c b/mm/mseal.c
> index 7a8ac66dc215..551b5ad52f4d 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -22,6 +22,7 @@ static bool range_contains_unmapped(unsigned long start, unsigned long end)
>  	unsigned long prev_end = start;
>  	struct vm_area_struct *vma;
>
> +	guard(rcu)();
>  	for_each_vma_range(vmi, vma, end) {
>  		if (vma->vm_start > prev_end)
>  			return true;
> @@ -43,6 +44,7 @@ static int __mseal_range(unsigned long start, unsigned long end)
>  	if (start > vma->vm_start)
>  		prev = vma;
>
> +	rcu_read_lock();
>  	for_each_vma_range(vmi, vma, end) {
>  		const unsigned long curr_start = max(vma->vm_start, start);
>  		const unsigned long curr_end = min(vma->vm_end, end);
> @@ -51,17 +53,30 @@ static int __mseal_range(unsigned long start, unsigned long end)
>  			vma_flags_t vma_flags = vma->flags;
>
>  			vma_flags_set(&vma_flags, VMA_SEALED_BIT);
> +			rcu_read_unlock();
>
>  			vma = vma_modify_flags(&vmi, prev, vma, curr_start,
>  					       curr_end, &vma_flags);
>  			if (IS_ERR(vma))
>  				return PTR_ERR(vma);
> +
> +			rcu_read_lock();
> +			/* The modify may have slept and merged, so re-lookup. */
> +			vma_iter_set(&vmi, curr_start);
> +			vma = vma_find(&vmi, curr_end);
> +			if (!vma) {
> +				rcu_read_unlock();
> +				return -ENOMEM;
> +			}
> +			rcu_read_unlock();
>  			vma_start_write(vma);
>  			vma_set_flags(vma, VMA_SEALED_BIT);
> +			rcu_read_lock();
>  		}
>
>  		prev = vma;
>  	}
> +	rcu_read_unlock();
>
>  	return 0;
>  }
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 23fb68fce000..464570b15d4a 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1134,6 +1134,7 @@ static int mwriteprotect_range(struct userfaultfd_ctx *ctx, unsigned long start,
>  		goto out_unlock;
>
>  	err = -ENOENT;
> +	rcu_read_lock();
>  	for_each_vma_range(vmi, dst_vma, end) {
>
>  		if (!userfaultfd_wp(dst_vma)) {
> @@ -1150,14 +1151,18 @@ static int mwriteprotect_range(struct userfaultfd_ctx *ctx, unsigned long start,
>
>  		_start = max(dst_vma->vm_start, start);
>  		_end = min(dst_vma->vm_end, end);
> +		rcu_read_unlock();
>
>  		err = uffd_wp_range(dst_vma, _start, _end - _start, enable_wp);
> +		rcu_read_lock();
> +		/* The iterator is still on a life node */
>
>  		/* Return 0 on success, <0 on failures */
>  		if (err < 0)
>  			break;
>  		err = 0;
>  	}
> +	rcu_read_unlock();
>  out_unlock:
>  	up_read(&ctx->map_changing_lock);
>  	mmap_read_unlock(dst_mm);
> @@ -2324,9 +2329,17 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx,
>  	if (vma->vm_start < start)
>  		prev = vma;
>
> +	rcu_read_lock();
>  	for_each_vma_range(vmi, vma, end) {
> +		rcu_read_unlock();
>  		cond_resched();
>
> +		rcu_read_lock();
> +		/* The cond_resched above may have slept, so re-lookup. */
> +		vma_iter_set(&vmi, vma->vm_start);
> +		vma = vma_find(&vmi, end);
> +		if (!vma)
> +			break;
>  		VM_WARN_ON_ONCE(!vma_can_userfault(vma, vm_flags, wp_async));
>  		VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx &&
>  				vma->vm_userfaultfd_ctx.ctx != ctx);
> @@ -2355,6 +2368,7 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx,
>  		new_vma_flags = vma->flags;
>  		vma_flags_clear_mask(&new_vma_flags, __VMA_UFFD_FLAGS);
>  		vma_flags_set_mask(&new_vma_flags, vma_flags);
> +		rcu_read_unlock();
>
>  		vma = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
>  					    &new_vma_flags,
> @@ -2368,15 +2382,31 @@ static int userfaultfd_register_range(struct userfaultfd_ctx *ctx,
>  		 * the next vma was merged into the current one and
>  		 * the current one has not been updated yet.
>  		 */
> +		rcu_read_lock();
> +		/* The modify may have slept and merged, so re-lookup. */
> +		vma_iter_set(&vmi, start);
> +		vma = vma_find(&vmi, end);
> +		if (!vma)
> +			break;
> +		rcu_read_unlock();
>  		userfaultfd_set_ctx(vma, ctx, vm_flags);
> +		rcu_read_lock();
>
> -		if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> +		if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma)) {
> +			rcu_read_unlock();
>  			hugetlb_unshare_all_pmds(vma);
> +			rcu_read_lock();
> +			vma_iter_set(&vmi, start);
> +			vma = vma_find(&vmi, end);
> +			if (!vma)
> +				break;
> +		}
>
>  skip:
>  		prev = vma;
>  		start = vma->vm_end;
>  	}
> +	rcu_read_unlock();
>
>  	return 0;
>  }
> @@ -3808,16 +3838,26 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>  	found = false;
>  	basic_ioctls = false;
>  	cur = vma;
> +	rcu_read_lock();
>  	do {
> +		rcu_read_unlock();
>  		cond_resched();
>
> +		rcu_read_lock();
> +		/* The cond_resched above may have slept, so re-lookup. */
> +		vma_iter_set(&vmi, cur->vm_start);
> +		cur = vma_find(&vmi, end);
> +		if (!cur)
> +			break;
>  		VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
>  				!!(cur->vm_flags & __VM_UFFD_FLAGS));
>
>  		/* check not compatible vmas */
>  		ret = -EINVAL;
> -		if (!vma_can_userfault(cur, vm_flags, wp_async))
> +		if (!vma_can_userfault(cur, vm_flags, wp_async)) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * RWP uses protnone as an access-tracking marker. PROT_NONE
> @@ -3827,8 +3867,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>  		 * mprotect() must still be unregisterable, so this is not
>  		 * part of vma_can_userfault().
>  		 */
> -		if ((vm_flags & VM_UFFD_RWP) && !vma_is_accessible(cur))
> +		if ((vm_flags & VM_UFFD_RWP) && !vma_is_accessible(cur)) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * UFFDIO_COPY will fill file holes even without
> @@ -3839,8 +3881,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>  		 * F_WRITE_SEAL can be taken until the vma is destroyed.
>  		 */
>  		ret = -EPERM;
> -		if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> +		if (unlikely(!(cur->vm_flags & VM_MAYWRITE))) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * If this vma contains ending address, and huge pages
> @@ -3852,11 +3896,15 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>
>  			ret = -EINVAL;
>
> -			if (end & (vma_hpagesize - 1))
> +			if (end & (vma_hpagesize - 1)) {
> +				rcu_read_unlock();
>  				goto out_unlock;
> +			}
>  		}
> -		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE))
> +		if ((vm_flags & VM_UFFD_WP) && !(cur->vm_flags & VM_MAYWRITE)) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * Check that this vma isn't already owned by a
> @@ -3866,8 +3914,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>  		 */
>  		ret = -EBUSY;
>  		if (cur->vm_userfaultfd_ctx.ctx &&
> -		    cur->vm_userfaultfd_ctx.ctx != ctx)
> +		    cur->vm_userfaultfd_ctx.ctx != ctx) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * Mode switches that drop VM_UFFD_WP or VM_UFFD_RWP would
> @@ -3876,8 +3926,10 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>  		 * into the other mode. Require an unregister first.
>  		 */
>  		if (cur->vm_userfaultfd_ctx.ctx == ctx &&
> -		    cur->vm_flags & (VM_UFFD_WP | VM_UFFD_RWP) & ~vm_flags)
> +		    cur->vm_flags & (VM_UFFD_WP | VM_UFFD_RWP) & ~vm_flags) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * Note vmas containing huge pages
> @@ -3887,6 +3939,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>
>  		found = true;
>  	} for_each_vma_range(vmi, cur, end);
> +	rcu_read_unlock();
>  	VM_WARN_ON_ONCE(!found);
>
>  	ret = userfaultfd_register_range(ctx, vma, vm_flags, start, end,
> @@ -3980,9 +4033,17 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>  	 */
>  	found = false;
>  	cur = vma;
> +	rcu_read_lock();
>  	do {
> +		rcu_read_unlock();
>  		cond_resched();
>
> +		rcu_read_lock();
> +		/* The cond_resched above may have slept, so re-lookup. */
> +		vma_iter_set(&vmi, cur->vm_start);
> +		cur = vma_find(&vmi, end);
> +		if (!cur)
> +			break;
>  		VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
>  				!!(cur->vm_flags & __VM_UFFD_FLAGS));
>
> @@ -3991,8 +4052,10 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>  		 * the one used for registration.
>  		 */
>  		if (cur->vm_userfaultfd_ctx.ctx &&
> -		    cur->vm_userfaultfd_ctx.ctx != ctx)
> +		    cur->vm_userfaultfd_ctx.ctx != ctx) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		/*
>  		 * Check not compatible vmas, not strictly required
> @@ -4001,11 +4064,14 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>  		 * provides for more strict behavior to notice
>  		 * unregistration errors.
>  		 */
> -		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> +		if (!vma_can_userfault(cur, cur->vm_flags, wp_async)) {
> +			rcu_read_unlock();
>  			goto out_unlock;
> +		}
>
>  		found = true;
>  	} for_each_vma_range(vmi, cur, end);
> +	rcu_read_unlock();
>  	VM_WARN_ON_ONCE(!found);
>
>  	vma_iter_set(&vmi, start);
> diff --git a/mm/vma.c b/mm/vma.c
> index 35e7a64855fa..e00b0cdc3d83 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -614,10 +614,8 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	validate_mm(vma->vm_mm);
>
>  	/* Success. */
> -	if (new_below)
> -		vma_next(vmi);
> -	else
> -		vma_prev(vmi);
> +	vma_iter_set(vmi, vma->vm_start);
> +	vma_find(vmi, ULONG_MAX);
>
>  	return 0;
>
> @@ -1573,7 +1571,12 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
>  #endif
>  	}
>
> -	vms->next = vma_next(vms->vmi);
> +	/*
> +	 * The loop's cached node may be the one a split's store deferred.
> +	 * Continue from the range end.
> +	 */
> +	vma_iter_set(vms->vmi, vms->end);
> +	vms->next = vma_find(vms->vmi, ULONG_MAX);
>  	if (vms->next)
>  		vms->unmap_end = vms->next->vm_start;
>
> --
> 2.54.0
>

--
Cheers, Lorenzo

Re: [PATCH] RCU safety for vma maple tree walks

[Liam R. Howlett]

On 26/08/31 07:35AM, Andi Kleen wrote:
> I ran into the following scenario in a slightly modified kernel:

...

> 
> That said I wasn't actually able to trigger it in a unmodified kernel
> so far with stress testing.
> 
> The following old unsolved syzkaller report has a similar signature,
> so maybe it was already seen:
> https://syzkaller.appspot.com/bug?id=4c5268fbb1d6d508a4c34dc425e2693d1ff9911a

Did you look at that syzbot?  He has 1 reply on-list pointing to the
discussion and remedy of the issue - which was debug validation code
running outside the rcu lock.

> 
> I guess in many cases where it happens for real you don't notice it
> if you don't have KASAN active.
> 
> The patch fixes up all callers to maintain the RCU reader lock
> regions correctly during the VMA walk. If they cannot be maintained the
> iterator is refreshed by a new VMA address lookup in a new region, unless
> it is proven safe not to.
> 
> In the cases where there is no sleeping it is strictly not needed
> because this scenario could not happen due the existing VMA locking.
> 
> But I fixed them too to not violate the maple tree iterator
> "rcu read lock or write lock" contract.
> 

Did you check this with lockdep?

Any access to the maple tree without holding the write lock or the rcu
read lock will cause lockdep to complain.

Likewise, any sleeping while holding the rcu read lock would produce
warnings.

Note that you do not need to hold the rcu read lock on the vma maple
tree if you have the write lock.  This looks to be the case for most (or
all?) of the cases you have below.

I suspect you have a locking issue in your modifications.


> The ones that do not strictly need it are: count_mm_mlocked_page_nr,
> remap_file_pages, range_contains_unmapped.
> 
> The ones that may sleep and clearly need it are: apply_vma_lock_flags,
> do_mprotect_pkey, remap_move, mseal_apply, mbind_range, userfaultfd
> register/unregister,
> mwriteprotect_range (doesn't sleep in the walk itself, but uses iterator
> after sleep)
> 
> In principle it could be optimized more, e.g. for example only do the
> re-lookups when actual sleeping happened. Some of it could be done
> with a new cond resched variant. But I tried to keep it minimal
> for now.
> 
> The patch survived most of LTP, the kernel mm selftests and
> my own stress tests. I didn't do any benchmarks.
> 
> For when it was introduced it's a complex area, but I believe the patch
> that originally added the problem was
> commit b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
> Then
> commit 17f1ae9b40c6 ("mm/vma: change munmap to use vma_munmap_struct() for
> accounting and surrounding vmas")
> extended the pattern to more places, and then later it was copied
> elsewhere too. I'm mentioning only the first below.
> 
> No cc stable so far, needs some discussion first.

Please send patches as RFC if they need discussion.

> 
> Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
> Assisted-by: omp:gpt-5.6-luna
> Signed-off-by: Andi Kleen <ak@kernel.org>

It looks like you have asked gtp to fix the locking issue you have
discovered by editing the exit_mmap() function.  I suspect this
functions complex locking dance has fooled things up on you.

If any of these issues were a problem, lockdep would be reporting about
missing locks and people would have hit these in the last 3 years (the
last syzbot report which is resolved).  Your scenario isn't that unusual
- most cloud vendors run machines flat out with as much ram used as
possible.

Thanks,
Liam

Re: [PATCH] RCU safety for vma maple tree walks

[Andi Kleen]

> 
> Did you check this with lockdep?

Yes all tests were running with lockdep, and it didn't trigger.

> 
> Any access to the maple tree without holding the write lock or the rcu
> read lock will cause lockdep to complain.

Hmm, seems that's not working for some reason?

The VM callers of for_each_vma clearly don't take rcu read locks.

> 
> Likewise, any sleeping while holding the rcu read lock would produce
> warnings.

Right but the walkers don't use the read lock.
 
> Note that you do not need to hold the rcu read lock on the vma maple
> tree if you have the write lock.  This looks to be the case for most (or
> all?) of the cases you have below.

Yes the write lock is hold (like I discussed).  The problem isn't parallel
modification but that splitting during the walk invalidates the
iterator caching if you sleep too. That's all purely on the local
thread. The only contribution from others is to cause more sleeping
due to memory pressure or rescheduling.

-Andi

Re: [PATCH] RCU safety for vma maple tree walks

[Liam R. Howlett]

On 26/08/31 09:52AM, Andi Kleen wrote:
> > 
> > Did you check this with lockdep?
> 
> Yes all tests were running with lockdep, and it didn't trigger.
> 
> > 
> > Any access to the maple tree without holding the write lock or the rcu
> > read lock will cause lockdep to complain.
> 
> Hmm, seems that's not working for some reason?
> 
> The VM callers of for_each_vma clearly don't take rcu read locks.

I can't say why it's not triggering for you, but I can tell you that
lockdep detects incorrect locking for me and others using the maple
tree.

You even pointed to a syzbot example of a report of an rcu locking issue
- which is resolved.  No unresolved issues by syzbot (which has lockdep
 working..) has been reported in any of the functions you have patched.

It is okay to not hold the rcu lock if you hold the write lock.

> 
> > 
> > Likewise, any sleeping while holding the rcu read lock would produce
> > warnings.
> 
> Right but the walkers don't use the read lock.

Every walker either holds the read or the write lock, please recheck the
code.

> > Note that you do not need to hold the rcu read lock on the vma maple
> > tree if you have the write lock.  This looks to be the case for most (or
> > all?) of the cases you have below.
> 
> Yes the write lock is hold (like I discussed).  The problem isn't parallel
> modification but that splitting during the walk invalidates the
> iterator caching if you sleep too. That's all purely on the local
> thread. The only contribution from others is to cause more sleeping
> due to memory pressure or rescheduling.

I'm going to state some things that will hopefully help us understand
our disconnect.

Parallel modifications are not possible.

Splits are writes.

Writes update the maple state.

Sleeping with the write lock means nothing writes.

Writers replace the nodes in the tree before marking old nodes as dead.

Writers mark the nodes dead before they are added to the rcu free list.

Writers return pointing to the new node, not the dead node.

Readers cannot sleep.

Readers that see a dead node restart the walk from the top of the tree
into a new node that is not dead.

Nodes cannot be freed until all readers which could have had a reference
have dropped the rcu read lock.

I am very confident you are doing something wrong in the code that is
not upstream.  If you have a reproducer with the upstream kernel, then I
can help you.

Thanks,
Liam