Re: [PATCH v2 3/4] mm/ksm: make break_ksm() more scalable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] mm/ksm: redundant ksm_walk_lock enum and helper in break_ksm()
- [Low] mm/ksm: non-const stack mm_walk_ops in break_ksm() contradicts commit log
--

commit 05d53e6a6817e77860f887304a4f4cd4936f2c61
Author: Xu Xin <xu.xin@linux.dev>

mm/ksm: make break_ksm() more scalable

This commit replaces the boolean lock_vma parameter of break_ksm() with a
KSM-local enum ksm_walk_lock and constructs struct mm_walk_ops on the stack
instead of selecting between two static const tables.

> diff --git a/mm/ksm.c b/mm/ksm.c
> index d4fc0b0965aa1..84d363dabdc8a 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -661,15 +661,25 @@ static int break_ksm_pmd_entry(pmd_t *pmdp, unsigned long addr, unsigned long en
>  	return found;
>  }
>  
> -static const struct mm_walk_ops break_ksm_ops = {
> -	.pmd_entry = break_ksm_pmd_entry,
> -	.walk_lock = PGWALK_RDLOCK,
> +/*
> + * Locking contexts in which break_ksm() may walk the page tables.
> + */
> +enum ksm_walk_lock {
> +	/* walk under mmap_read_lock() */
> +	KSM_WALK_MMAP_RDLOCK,
> +	/* walk under mmap_write_lock() */
> +	KSM_WALK_MMAP_WRLOCK,
>  };
>  
> -static const struct mm_walk_ops break_ksm_lock_vma_ops = {
> -	.pmd_entry = break_ksm_pmd_entry,
> -	.walk_lock = PGWALK_WRLOCK,
> -};
> +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;
> +	default:
> +		return PGWALK_RDLOCK;
> +	}
> +}

[Severity: Low]
This isn't a bug, but is enum ksm_walk_lock and ksm_walk_lock_to_pgwalk()
needed here?

include/linux/pagewalk.h is already included in mm/ksm.c and defines enum
page_walk_lock (PGWALK_RDLOCK, PGWALK_WRLOCK, PGWALK_VMA_RDLOCK_VERIFY),
which is already used directly elsewhere in mm/ksm.c. Since break_ksm() only
uses walk_lock to populate ops.walk_lock, could break_ksm() take enum
page_walk_lock directly instead of introducing a 1:1 wrapper enum and
conversion helper?

Also, the comment on KSM_WALK_MMAP_WRLOCK says "walk under mmap_write_lock()",
whereas mapping it to PGWALK_WRLOCK causes process_vma_walk_lock() to acquire
the VMA write lock via vma_start_write(vma) rather than only asserting that
mmap_write_lock() is held.

>  /*
>   * Though it's very tempting to unmerge rmap_items from stable tree rather
> @@ -697,17 +707,19 @@ static const struct mm_walk_ops break_ksm_lock_vma_ops = {
>   * protection keys here anyway.
>   */
>  static int break_ksm(struct vm_area_struct *vma, unsigned long addr,
> -		unsigned long end, bool lock_vma)
> +		unsigned long end, enum ksm_walk_lock walk_lock)
>  {
>  	vm_fault_t ret = 0;
> -	const struct mm_walk_ops *ops = lock_vma ?
> -				&break_ksm_lock_vma_ops : &break_ksm_ops;
> +	struct mm_walk_ops ops = {
> +		.pmd_entry = break_ksm_pmd_entry,
> +		.walk_lock = ksm_walk_lock_to_pgwalk(walk_lock),
> +	};

[Severity: Low]
The commit message states:

  break_ksm() currently takes a boolean 'lock_vma' to select between two
  static mm_walk_ops tables.  This is fragile: the ops struct is full of
  function pointers and making it mutable (to switch walk_lock at runtime)
  turns it into a juicy target for attackers.

  Replace the boolean with a KSM-local enum ksm_walk_lock and build the
  mm_walk_ops on the stack, so the table stays const and there is no
  shared mutable state.

Previously, break_ksm_ops and break_ksm_lock_vma_ops were declared as static
const struct mm_walk_ops in .rodata, whereas this change declares struct
mm_walk_ops ops on the writable stack without const and reinitializes it on
every break_ksm() call.

Does moving the function pointer table from static const storage in .rodata
onto the stack without const match the security rationale in the commit
message, or should break_ksm() either keep selecting between static const
struct mm_walk_ops tables (as done in fs/proc/task_mmu.c) or at least mark
the stack struct const?

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