Re: [PATCH v2 4/4] ksm: add find_mergeable_vma_locked() to use per-VMA locking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] mm: ksm: race with exit_mmap() in find_mergeable_vma_locked() and __ksm_exit()
- [High] mm: ksm: missing FAULT_FLAG_VMA_LOCK in break_ksm() under per-VMA read lock
- [Low] mm: ksm: unmarked vma->anon_vma read in find_mergeable_vma_locked()
- [Low] mm: ksm: missing kernel-doc params for find_mergeable_vma_locked()
--

commit 984120154ab3ef7af44dc3dbb32b8522a336aefe
Author: Xu Xin <xu.xin@linux.dev>

ksm: add find_mergeable_vma_locked() to use per-VMA locking

This commit introduces find_mergeable_vma_locked() to look up and read-lock
VM_MERGEABLE VMAs using per-VMA locks instead of mmap_read_lock() across KSM
page-scanning and merging paths. It also updates __ksm_exit() to write-lock
each VMA under mmap_write_lock() to synchronize with in-flight per-VMA readers
when a process exits.

Link: https://lore.kernel.org/all/20260909023507.1408925-1-xialonglong2025@163.com/
Link: https://lore.kernel.org/all/91cf8129-d705-4ebd-b420-129bab7ed261@kernel.org/
Link: https://lore.kernel.org/all/20260911162242418ODBbr9wspxxIXDS8uvpeZ@zte.com.cn/

> diff --git a/mm/ksm.c b/mm/ksm.c
> index 84d363dabdc8a..64f9e803b99e2 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -669,6 +669,8 @@ enum ksm_walk_lock {
>  	KSM_WALK_MMAP_RDLOCK,
>  	/* walk under mmap_write_lock() */
>  	KSM_WALK_MMAP_WRLOCK,
> +	/* walk with the vma already read-locked (per-VMA locking) */
> +	KSM_WALK_VMA_RDLOCK,
>  };
>  
>  static enum page_walk_lock ksm_walk_lock_to_pgwalk(enum ksm_walk_lock lock)
> @@ -676,6 +678,8 @@ static enum page_walk_lock ksm_walk_lock_to_pgwalk(enum ksm_walk_lock lock)
>  	switch (lock) {
>  	case KSM_WALK_MMAP_WRLOCK:
>  		return PGWALK_WRLOCK;
> +	case KSM_WALK_VMA_RDLOCK:
> +		return PGWALK_VMA_RDLOCK_VERIFY;
>  	default:
>  		return PGWALK_RDLOCK;
>  	}
> @@ -784,15 +788,36 @@ static bool vma_ksm_compatible(struct vm_area_struct *vma)
>  	return ksm_compatible(vma->vm_file, vma->flags);
>  }
>  
> -static struct vm_area_struct *find_mergeable_vma(struct mm_struct *mm,
> -		unsigned long addr)
> +/**
> + * find_mergeable_vma_locked() - Find the VMA covering 'address' which is
> + * VM_MERGEABLE and read-lock it by per-VMA locks. Please use vma_end_read()
> + * to unlock vma after finishing reading the VMA (non-NULL).
> + *
> + * Return: If a VMA exists which spans @address, return that VMA, read-locked.
> + * If no VMA is mapped there or, very unlikely, a reference count overflow
> + * occurred, return NULL, and no read-locked.
> + *
> + * IMPORTANT: If a VMA exists but is not VM_MERGEABLE or has no anon_vma,
> + * this function releases the per-VMA read lock before returning NULL.
> + * Callers must NOT call vma_end_read() on a NULL return value.
> + */
> +static struct vm_area_struct *find_mergeable_vma_locked(struct mm_struct *mm,
> +		unsigned long address)

[Severity: Low]
This isn't a bug, but since this comment above find_mergeable_vma_locked() in
mm/ksm.c starts with the /** kernel-doc marker, won't scripts/kernel-doc (when
building with W=1) warn about the missing @mm and @address parameter
descriptions?

>  {
>  	struct vm_area_struct *vma;
> +
>  	if (ksm_test_exit(mm))
>  		return NULL;
> -	vma = vma_lookup(mm, addr);
> -	if (!vma || !(vma->vm_flags & VM_MERGEABLE) || !vma_has_anon_rmap(vma))
> +
> +	vma = vma_start_read_unlocked(mm, address);
> +	if (!vma)
>  		return NULL;
> +
> +	if (!(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma) {

[Severity: Low]
In find_mergeable_vma_locked() in mm/ksm.c, replacing !vma_has_anon_rmap(vma)
with a plain !vma->anon_vma read under the per-VMA read lock drops the
data_race() annotation and vma_assert_stabilised() check from
vma_has_anon_rmap() in mm/vma.h:

mm/vma.h:vma_has_anon_rmap() {
	if (vma_is_attached(vma))
		vma_assert_stabilised(vma);
	/* KCSAN gets confused about the optimistic check. Silence it. */
	return data_race(vma->anon_vma);
}

When another thread concurrently faults in the first anonymous page on the
same VMA and initializes vma->anon_vma in __anon_vma_prepare() under
mm->page_table_lock, won't this trigger a KCSAN data-race warning? Should
find_mergeable_vma_locked() continue to use !vma_has_anon_rmap(vma) here?

> +		vma_end_read(vma);
> +		return NULL;
> +	}
> +
>  	return vma;
>  }
>  
> @@ -818,11 +843,12 @@ static void break_cow(struct ksm_rmap_item *rmap_item)
>  	 */
>  	rmap_item->linear_page_index = 0;
>  
> -	mmap_read_lock(mm);
> -	vma = find_mergeable_vma(mm, addr);
> -	if (vma)
> -		break_ksm(vma, addr, addr + PAGE_SIZE, KSM_WALK_MMAP_RDLOCK);
> -	mmap_read_unlock(mm);
> +	vma = find_mergeable_vma_locked(mm, addr);
> +	if (!vma)
> +		return;
> +
> +	break_ksm(vma, addr, addr + PAGE_SIZE, KSM_WALK_VMA_RDLOCK);
> +	vma_end_read(vma);
>  }

[Severity: High]
When break_cow() in mm/ksm.c calls break_ksm() with KSM_WALK_VMA_RDLOCK, only
the per-VMA read lock is held, without mm->mmap_lock. However, break_ksm()
still calls handle_mm_fault() without FAULT_FLAG_VMA_LOCK:

mm/ksm.c:break_ksm() {
	...
	do {
		int ksm_page;

		cond_resched();
		ksm_page = walk_page_range_vma(vma, addr, end, &ops, &addr);
		if (ksm_page <= 0)
			return ksm_page;
		ret = handle_mm_fault(vma, addr,
				      FAULT_FLAG_UNSHARE | FAULT_FLAG_REMOTE,
				      NULL);
	} while (!(ret & (VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | VM_FAULT_OOM)));
}

Because break_ksm_pmd_entry() drops the PTE lock before returning, the PTE at
addr can be modified (for example, zapped by concurrent MADV_DONTNEED or
truncation, or turned into a device-private swap entry) before
handle_pte_fault() inspects it.

Without FAULT_FLAG_VMA_LOCK set in vmf->flags, handle_mm_fault() and its
callees assume mm->mmap_lock is held:

1. In mm/memory.c, vmf_can_call_fault() checks !(vmf->flags &
   FAULT_FLAG_VMA_LOCK) and allows do_read_fault() to invoke
   vma->vm_ops->fault() on a ksm_compatible() MAP_PRIVATE file-backed VMA
   without holding mm->mmap_lock.

2. In include/linux/mm.h, assert_fault_locked() checks
   mmap_assert_locked(vmf->vma->vm_mm) when FAULT_FLAG_VMA_LOCK is not set,
   and release_fault_lock() calls mmap_read_unlock() on the unheld
   mm->mmap_lock (e.g. if handle_userfault() or migration entry waiting is
   reached).

3. If FAULT_FLAG_VMA_LOCK were passed, any path returning VM_FAULT_RETRY
   would release the per-VMA read lock via vma_end_read(vma), which neither
   the loop in break_ksm() nor the unconditional vma_end_read(vma) in
   break_cow() accounts for.

Can break_ksm() safely call handle_mm_fault() under only a per-VMA read lock?

[ ... ]

> @@ -3153,7 +3178,20 @@ void __ksm_exit(struct mm_struct *mm)
>  		mm_flags_clear(MMF_VM_MERGEABLE, mm);
>  		mmdrop(mm);
>  	} else if (mm_slot) {
> +		/*
> +		 * ksmd may be walking this mm's page tables under a per-VMA
> +		 * read lock (see find_mergeable_vma_locked()).  A plain
> +		 * mmap_write_lock() does not exclude such readers, so also
> +		 * write-lock every VMA: this blocks until any in-flight ksmd
> +		 * walk drops its per-VMA read lock, and prevents new ones
> +		 * from starting (vma_start_read_unlocked() backs out when a
> +		 * VMA is write-locked).  The per-VMA write locks are released
> +		 * by mmap_write_unlock() via vma_end_write_all().
> +		 */
>  		mmap_write_lock(mm);
> +		VMA_ITERATOR(vmi, mm, 0);
> +		for_each_vma(vmi, vma)
> +			vma_start_write(vma);
>  		mmap_write_unlock(mm);
>  	}

[Severity: Critical]
Does this synchronization in __ksm_exit() in mm/ksm.c actually prevent ksmd
from acquiring a per-VMA read lock in find_mergeable_vma_locked() and racing
with exit_mmap()?

Unlike normal callers of vma_start_read_unlocked(), ksmd only holds mm_count
(via mmgrab()), not mm_users. Previously, callers took mmap_read_lock(mm)
first and then checked ksm_test_exit(mm) inside find_mergeable_vma() under
mmap_read_lock(mm), which serialized against mmap_write_lock(mm) in
__ksm_exit().

With this patch, find_mergeable_vma_locked() in mm/ksm.c checks
ksm_test_exit(mm) locklessly before calling vma_start_read_unlocked(mm,
address), and never re-checks ksm_test_exit(mm) after acquiring the per-VMA
read lock:

mm/ksm.c:find_mergeable_vma_locked() {
	if (ksm_test_exit(mm))
		return NULL;

	vma = vma_start_read_unlocked(mm, address);
	if (!vma)
		return NULL;
	...
}

Contrary to the comment in __ksm_exit(), vma_start_read_unlocked() in
mm/mmap_lock.c does not back out when a VMA is write-locked; when
lock_vma_under_rcu() returns NULL, it enters the slow path and blocks on
mmap_read_lock(mm):

mm/mmap_lock.c:vma_start_read_unlocked() {
	vma = lock_vma_under_rcu(mm, address);
	if (vma)
		return vma;

	/* Slow path: preclude VMA writers by temporarily getting mmap read lock. */
	mmap_read_lock(mm);
	vma = vma_lookup(mm, address);
	if (vma && !vma_start_read_locked(vma))
		vma = NULL;
	mmap_read_unlock(mm);

	return vma;
}

And in __ksm_exit(), mmap_write_unlock(mm) calls vma_end_write_all(mm), which
immediately releases all of the per-VMA write locks just acquired.

If mm->mm_users drops to 0 right after find_mergeable_vma_locked() passes
ksm_test_exit(mm) (in get_mergeable_page(), try_to_merge_with_zero_page(),
try_to_merge_with_ksm_page(), or break_cow()):

1. If ksmd calls vma_start_read_unlocked() while __ksm_exit() holds
   mmap_write_lock(mm), lock_vma_under_rcu() fails and ksmd waits on
   mmap_read_lock(mm) in the slow path. As soon as __ksm_exit() calls
   mmap_write_unlock(mm) (which also calls vma_end_write_all(mm)), ksmd
   acquires mmap_read_lock(mm), looks up the VMA, and acquires the per-VMA
   read lock via vma_start_read_locked(vma). Or if ksmd enters
   vma_start_read_unlocked() right after __ksm_exit() calls
   mmap_write_unlock(mm), lock_vma_under_rcu() succeeds directly because the
   per-VMA write locks were already released.

   Because find_mergeable_vma_locked() does not re-check ksm_test_exit(mm),
   ksmd proceeds to walk and modify page tables (and invoke MMU notifiers)
   under only the per-VMA read lock after __ksm_exit() and
   mmu_notifier_release() have finished, racing with exit_mmap() running
   unmap_vmas() and free_pgtables() (which frees page tables before
   tear_down_vmas() detaches the VMAs).

2. Because mm_users is not pinned across vma_start_read_unlocked(),
   exit_mmap() in mm/mmap.c can run concurrently with
   vma_start_read_unlocked():

mm/mmap.c:exit_mmap() {
	...
	mmap_write_lock(mm);
	unmap.mm_wr_locked = true;
	mt_clear_in_rcu(&mm->mm_mt);
	unmap_pgtable_init(&unmap, &vmi);
	free_pgtables(&tlb, &unmap);
	tlb_finish_mmu(&tlb);
	nr_accounted = tear_down_vmas(mm, &vmi, vma, ULONG_MAX);
destroy:
	__mt_destroy(&mm->mm_mt);
	mmap_write_unlock(mm);
	...
}

   In exit_mmap(), mt_clear_in_rcu(&mm->mm_mt) clears MT_FLAGS_USE_RCU and
   __mt_destroy(&mm->mm_mt) frees maple tree nodes synchronously without RCU
   callbacks, while lock_vma_under_rcu() -> mas_walk() in ksmd may still be
   walking mm->mm_mt under rcu_read_lock(), leading to a use-after-free on
   maple tree nodes (or a maple tree lockdep warning in the slow path's
   vma_lookup() after __mt_destroy() clears MT_FLAGS_LOCK_EXTERN).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102132416.300084-1-xu.xin@linux.dev?part=4