Re: [PATCH v2 2/4] mm: make folio_walk_start()'s locking asserts scalable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mm: inaccurate commit message on write-locking in folio_walk_start()
- [Low] mm: folio_walk_start() duplicates vma_assert_stabilised() via walk_lock
- [Low] mm: missing kernel-doc and private placement for folio_walk::walk_lock
--

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

mm: make folio_walk_start()'s locking asserts scalable

This commit adds a walk_lock member to struct folio_walk and updates
folio_walk_start() and its callers to specify locking assertion modes for
mmap_lock and per-VMA locks.

>     mm: make folio_walk_start()'s locking asserts scalable
>     
>     Add an additional member 'walk_lock' to folio_walk to indicate
>     locking requirements for the walk. Similar to commit 49b0638502da0
>     ("mm: enable page walking API to lock vmas during the walk").
>     But no change is made on any existing locking behavior,
>     all existing folio_walk_start() callers are currently still
>     under mmap_read_lock() protection.
>     
>     This change is prepared for the later patch to enable VMA locking
>     asserts. folio_walk_start now operate under write-locked mmap_lock.
>     With introduction of vma locks at the next patch, the vmas have to
>     be locked as well during such walks to prevent concurrent page
>     faults in these areas.
>     
>     No functional change intended.
>     
>     Signed-off-by: Xu Xin <xu.xin@linux.dev>

[Severity: Medium]
The second paragraph states that "folio_walk_start now operate under
write-locked mmap_lock" and that VMAs have to be locked during such walks.
Is this description accurate?

In mm/pagewalk.c, folio_walk_start() only expects PGWALK_RDLOCK or
PGWALK_VMA_RDLOCK_VERIFY and emits a warning on write-lock modes:

	if (fw->walk_lock != PGWALK_RDLOCK && fw->walk_lock != PGWALK_VMA_RDLOCK_VERIFY)
		WARN_ONCE(1, "walk_lock is not expected!\n");

Could the commit message be updated to reflect that folio_walk_start() only
verifies read-stabilized mmap or VMA locks?

[ ... ]

> diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
> index 1c397be0d092a..455428eaf1fc0 100644
> --- a/include/linux/pagewalk.h
> +++ b/include/linux/pagewalk.h
> @@ -184,6 +184,7 @@ struct folio_walk {
>  	};
>  	/* private */
>  	spinlock_t *ptl;
> +	enum page_walk_lock walk_lock;
>  };

[Severity: Low]
This isn't a bug, but adding walk_lock to struct folio_walk without a
@walk_lock entry in the kernel-doc comment above struct folio_walk triggers a
scripts/kernel-doc warning ("struct member 'walk_lock' not described in
'folio_walk'"), because the /* private */ comment does not use the
/* private: */ syntax recognized by kernel-doc.

Also, since callers of folio_walk_start() are required to initialize
fw.walk_lock before the call, should walk_lock be placed in the public
section of struct folio_walk rather than under /* private */, and should the
kernel-doc for folio_walk_start() in mm/pagewalk.c (which currently states
"@fw: filled with information on success." and "The mmap lock must be held in
read mode.") be updated to match?

[ ... ]

> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index e6493bbe6919e..7d85289fcb875 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -910,7 +910,15 @@ struct folio *folio_walk_start(struct folio_walk *fw,
>  	pgd_t *pgdp;
>  	p4d_t *p4dp;
>  
> -	mmap_assert_locked(vma->vm_mm);
> +	/*
> +	 * Other locking modes except for mmap or vma read locking are not
> +	 * expected.
> +	 */
> +	if (fw->walk_lock != PGWALK_RDLOCK && fw->walk_lock != PGWALK_VMA_RDLOCK_VERIFY)
> +		WARN_ONCE(1, "walk_lock is not expected!\n");
> +	process_mm_walk_lock(vma->vm_mm, fw->walk_lock);
> +	process_vma_walk_lock(vma, fw->walk_lock);

[Severity: Low]
If a caller passes an unexpected walk_lock value (such as PGWALK_WRLOCK) or
leaves struct folio_walk uninitialized, folio_walk_start() triggers
WARN_ONCE() here, but then still calls process_mm_walk_lock() and
process_vma_walk_lock(), which for PGWALK_WRLOCK will call
vma_start_write(vma).

More broadly, since folio_walk_start() only permits mmap or VMA read-lock
verification, could folio_walk_start() simply call vma_assert_stabilised(vma)
from include/linux/mmap_lock.h instead of adding walk_lock to struct
folio_walk and requiring every caller to initialize it:

static inline void vma_assert_stabilised(const struct vm_area_struct *vma)
{
	if (IS_ENABLED(CONFIG_LOCKDEP)) {
		if (lockdep_is_held(&vma->vm_mm->mmap_lock))
			return;
	} else {
		if (rwsem_is_locked(&vma->vm_mm->mmap_lock))
			return;
	}

	vma_assert_locked(vma);
}

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