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

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

[kernel test robot]

:::::: 
:::::: Manual check reason: "linux-review patch is more than 7 days old, verify it wasn't already superseded"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20261102133103.302832-1-xu.xin@linux.dev>
References: <20261102133103.302832-1-xu.xin@linux.dev>
TO: xu.xin@linux.dev
TO: xu.xin@linux.dev
CC: acme@kernel.org
CC: adrian.hunter@intel.com
CC: agordeev@linux.ibm.com
CC: akpm@linux-foundation.org
CC: alexander.shishkin@linux.intel.com
CC: apopple@nvidia.com
CC: baohua@kernel.org
CC: baolin.wang@linux.alibaba.com
CC: borntraeger@linux.ibm.com
CC: byungchul@sk.com
CC: chengming.zhou@linux.dev
CC: david@kernel.org
CC: dev.jain@arm.com
CC: gerald.schaefer@linux.ibm.com
CC: gor@linux.ibm.com
CC: gourry@gourry.net
CC: harry@kernel.org
CC: hca@linux.ibm.com
CC: irogers@google.com
CC: james.clark@linaro.org
CC: jannh@google.com
CC: jolsa@kernel.org
CC: joshua.hahnjy@gmail.com
CC: kas@kernel.org
CC: lance.yang@linux.dev
CC: liam@infradead.org
CC: linux-kernel@vger.kernel.org
CC: linux-mm@kvack.org
CC: linux-s390@vger.kernel.org

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/xu-xin-linux-dev/mm-pagewalk-delete-the-unused-member/20261102-212844
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20261102133103.302832-1-xu.xin%40linux.dev
patch subject: [PATCH v2 4/4] ksm: add find_mergeable_vma_locked() to use per-VMA locking
:::::: branch date: 7 days ago
:::::: commit date: 7 days ago
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20261110/202611100159.ZLtmDECp-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 16.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261110/202611100159.ZLtmDECp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202611100159.ZLtmDECp-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: mm/ksm.c:805 function parameter 'mm' not described in 'find_mergeable_vma_locked'
>> Warning: mm/ksm.c:805 function parameter 'address' not described in 'find_mergeable_vma_locked'
>> Warning: mm/ksm.c:805 function parameter 'mm' not described in 'find_mergeable_vma_locked'
>> Warning: mm/ksm.c:805 function parameter 'address' not described in 'find_mergeable_vma_locked'

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 0/4] mm/ksm: use per-VMA locking to reduce mmap_lock contention

[xu.xin]

From: Xu Xin <xu.xin@linux.dev>

KSM scans VM_MERGEABLE VMAs and currently protects each scan with
mmap_read_lock(). Per-VMA locking allows KSM to read-lock only the VMA
it is actually interested in, so that unrelated mmap()/munmap() activity
in the same mm no longer blocks ksmd.

This series is organized as follows:

  Patch 1 removes an unused 'vma' member from struct folio_walk.  It has
  never been used since its introduction and is pure cleanup.

  Patch 2 adds a 'walk_lock' member to struct folio_walk and extends
  folio_walk_start() to assert the required locking mode.  Existing
  callers are converted to pass PGWALK_RDLOCK, so there is no functional
  change.  This prepares folio_walk_start() for callers that hold a
  per-VMA read lock instead of mmap_read_lock(), which is needed by the
  Patch 4. No functional change.

  Patch 3 replaces the boolean 'lock_vma' argument of break_ksm() with a
  KSM-local enum ksm_walk_lock and builds the mm_walk_ops on the stack,
  so the function-pointer table stays const and there is no shared
  mutable state.  No functional change.

  Patch 4 introduces find_mergeable_vma_locked(), which uses the
  universal per-VMA locking helper vma_start_read_unlocked() to look up
  and read-lock a VM_MERGEABLE VMA without taking mmap_read_lock().  All
  KSM call sites that previously used find_mergeable_vma() under
  mmap_read_lock() are converted to the new helper, and the locking in
  get_mergeable_page() is switched to PGWALK_VMA_RDLOCK_VERIFY so that
  folio_walk_start() can verify the per-VMA lock is held.  Because ksmd
  no longer holds mmap_read_lock() while walking, __ksm_exit() must also
  write-lock every VMA (in addition to mmap_write_lock()) to keep
  serializing against an in-flight ksmd walk before the page tables are
  freed.

A microbenchmark was run to measure the time KSM takes to merge a
victim region under mmap_lock contention. Under interference from 4 churner
threads, the merge time of the per-VMA KSM-optimized kernel is
significantly reduced by 50%.

Changelog:
  v2:
  - Patch 1: rewrote the commit message to explain the introducing
    commit, how the VMA is actually used, and the audit performed;
    dropped the Fixes: tag.

  - Patch 3: 
    * replaced the boolean 'lock_vma' with a KSM-local enum
    ksm_walk_lock instead of exposing the internal enum page_walk_lock
    as a parameter;
    * build the mm_walk_ops on the stack rather than
    mutating a global struct, so the function-pointer table stays const
    and there is no shared mutable state; rewrote the commit message.

  - Patch 4:
    * Fixed a per-VMA read-lock leak in find_mergeable_vma_locked()
    when the VMA is not mergeable;
    made __ksm_exit() write-lock every VMA (vma_start_write()) in
    addition to mmap_write_lock(), so that it still serializes against a
    ksmd walk that holds only a per-VMA read lock.

Xu Xin (4):
  mm/pagewalk: delete the unused member
  mm: make folio_walk_start()'s locking asserts scalable
  mm/ksm: make break_ksm() more scalable
  ksm: add find_mergeable_vma_locked() to use per-VMA locking

 arch/s390/mm/fault.c     |   4 +-
 include/linux/pagewalk.h |   2 +-
 kernel/events/uprobes.c  |   4 +-
 mm/huge_memory.c         |   4 +-
 mm/ksm.c                 | 124 +++++++++++++++++++++++++++------------
 mm/migrate.c             |   8 ++-
 mm/pagewalk.c            |  10 +++-
 mm/rmap.c                |   4 +-
 8 files changed, 116 insertions(+), 44 deletions(-)

--
2.34.1

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

[xu.xin]

From: Xu Xin <xu.xin@linux.dev>

Longlong Xia reported that get_mergeable_page() takes mmap_read_lock()
on every merge, which becomes a contention point under load [1]. David
Hildenbrand suggested switching to per-VMA locking to avoid the
process-wide mmap lock [2].

Add find_mergeable_vma_locked(), which is similar to find_tcp_vma(),
using the universal per-VMA locking helper vma_start_read_unlocked() to
look up and read-lock a VM_MERGEABLE VMA without taking
mmap_read_lock().  All KSM call sites that previously used
find_mergeable_vma() under mmap_read_lock() are converted to the new
helper, and the locking in get_mergeable_page() is switched to
PGWALK_VMA_RDLOCK_VERIFY so that folio_walk_start() can verify the
per-VMA lock is held.

Switching to per-VMA locking also changes how ksmd is synchronized with
a process that is exiting.  __ksm_exit() previously relied on a plain
mmap_write_lock() to serialize against ksmd, but that no longer blocks
a ksmd walk that holds only a per-VMA read lock.  So __ksm_exit() now
also write-locks every VMA (vma_start_write()) while holding
mmap_write_lock(): this blocks until any in-flight ksmd walk drops its
per-VMA read lock, and prevents new ones from starting, since
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().  This is safe because __mmput() runs ksm_exit()
after mm_users has gone to zero and before exit_mmap() frees the page
tables, so any new ksmd walk backs out via ksm_test_exit().

Performance
===========
As a test, I construct a testcase which follows the approach: Create
one victim and several churner threads sharing one mm_struct [3]; The
victim registers a 32 MiB anonymous VM_MERGEABLE region containing
8192 pages totally:churners hammer mmap_lock via mmap/munmap
repeatedly; ksmd merges the victim's pages; Compare latency baseline
VS this per-VMA patch.

                 Before patched         After Patched
0 churner:     1.627 seconds          1.426 seconds
4 churners:    72.45 seconds          36.61 seconds

In conclusion, when no mmap_lock contention (0 churner), there is little
difference between the baseline kernel and the per-VMA optimized kernel;
But under interference from 4 churner threads, the merge time of the
per-VMA KSM-optimized kernel is significantly reduced by 50%.

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

Reported-by: Longlong Xia <xialonglong@kylinos.cn>
Signed-off-by: Xu Xin <xu.xin@linux.dev>
---
 mm/ksm.c | 82 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 22 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 84d363dabdc8..64f9e803b99e 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)
 {
 	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) {
+		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);
 }
 
 static struct page *get_mergeable_page(struct ksm_rmap_item *rmap_item)
@@ -833,13 +859,12 @@ static struct page *get_mergeable_page(struct ksm_rmap_item *rmap_item)
 	struct page *page = NULL;
 	struct folio *folio;
 	struct folio_walk fw = {
-		.walk_lock  = PGWALK_RDLOCK,
+		.walk_lock  = PGWALK_VMA_RDLOCK_VERIFY,
 	};
 
-	mmap_read_lock(mm);
-	vma = find_mergeable_vma(mm, addr);
+	vma = find_mergeable_vma_locked(mm, addr);
 	if (!vma)
-		goto out;
+		return NULL;
 
 	folio = folio_walk_start(&fw, vma, addr, 0);
 	if (folio) {
@@ -850,12 +875,12 @@ static struct page *get_mergeable_page(struct ksm_rmap_item *rmap_item)
 		}
 		folio_walk_end(&fw, vma);
 	}
-out:
+
 	if (page) {
 		flush_anon_page(vma, page, addr);
 		flush_dcache_page(page);
 	}
-	mmap_read_unlock(mm);
+	vma_end_read(vma);
 	return page;
 }
 
@@ -1596,14 +1621,14 @@ static int try_to_merge_with_zero_page(struct ksm_rmap_item *rmap_item,
 	if (ksm_use_zero_pages && (rmap_item->oldchecksum == zero_checksum)) {
 		struct vm_area_struct *vma;
 
-		mmap_read_lock(mm);
-		vma = find_mergeable_vma(mm, rmap_item->address);
+		vma = find_mergeable_vma_locked(mm, rmap_item->address);
 		if (vma) {
 			err = try_to_merge_one_page(vma, page,
 					ZERO_PAGE(rmap_item->address));
 			trace_ksm_merge_one_page(
 				page_to_pfn(ZERO_PAGE(rmap_item->address)),
 				rmap_item, mm, err);
+			vma_end_read(vma);
 		} else {
 			/*
 			 * If the vma is out of date, we do not need to
@@ -1611,7 +1636,6 @@ static int try_to_merge_with_zero_page(struct ksm_rmap_item *rmap_item,
 			 */
 			err = 0;
 		}
-		mmap_read_unlock(mm);
 	}
 
 	return err;
@@ -1630,10 +1654,9 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
 	struct vm_area_struct *vma;
 	int err = -EFAULT;
 
-	mmap_read_lock(mm);
-	vma = find_mergeable_vma(mm, rmap_item->address);
+	vma = find_mergeable_vma_locked(mm, rmap_item->address);
 	if (!vma)
-		goto out;
+		goto out_trace;
 
 	err = try_to_merge_one_page(vma, page, kpage);
 	if (err)
@@ -1653,7 +1676,8 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
 	rmap_item->linear_page_index = linear_anon_page_index(vma, rmap_item->address);
 	get_anon_vma(vma->anon_vma);
 out:
-	mmap_read_unlock(mm);
+	vma_end_read(vma);
+out_trace:
 	trace_ksm_merge_with_ksm_page(kpage, page_to_pfn(kpage ? kpage : page),
 				rmap_item, mm, err);
 	return err;
@@ -3119,6 +3143,7 @@ void __ksm_exit(struct mm_struct *mm)
 {
 	struct ksm_mm_slot *mm_slot = NULL;
 	struct mm_slot *slot;
+	struct vm_area_struct *vma;
 	int easy_to_free = 0;
 
 	/*
@@ -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);
 	}
 
-- 
2.34.1

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

[sashiko-bot]

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