Re: [PATCH v2] mm: implement and use vma_anon_tracked(), silence KCSAN

[Pedro Falcato]

On Thu, Sep 10, 2026 at 02:18:35PM +0100, Lorenzo Stoakes (ARM) wrote:
> Provide a function to abstract the common task of checking whether
> a VMA is faulted in or not.
> 
> A VMA or mmap lock must be held when calling this function. For an attached
> VMA the transitions between unfaulted/faulted state are:
> 
>        Transition        |              VMA/mmap Lock state
>  ------------------------|-----------------------------------------------
>    unfaulted to faulted  | write lock OR read lock + mm->page_table_lock
>    faulted to unfaulted  | write lock
> 
> So vma_anon_tracked() never provides a false positive (the lock precludes
> it), but if only a read lock is held, a negative result must be re-checked
> with mm->page_table_lock held.
> 
> Detached VMAs cannot be concurrently manipulated as they are removed from
> the maple tree so require no guarantees.
> 
> Use data_race() to silence KCSAN about non-existent data races between
> concurrent vma->anon_vma read/write on optimistic fault tests.
> 
> Also while here, const-ify vma_is_attached(), vma_assert_stabilised() and
> dependants.
> 
> Finally, update the core VMA merge/split, rmap, mremap, KSM and fault
> preparation callers which test vma->anon_vma directly to use
> vma_anon_tracked() instead.
> 
> Note that the lockless read in reusable_anon_vma() is doing more than
> checking whether the VMA is faulted - it is returning the anon_vma to be
> used on fault, so this check is not altered.
> 
> There is one odd one out - file_backed_vma_is_retractable() - which holds
> neither a VMA nor mmap lock and is stabilised by the file rmap lock only.
> 
> Therefore just add a comment to explain why the direct vma->anon_vma check
> is required.
> 
> Reported-by: Guilherme Giacomo Simoes <trintaeoitogc@xxxxxxxxx>
> Closes: https://lore.kernel.org/all/20260829100034.423064-1-trintaeoitogc@xxxxxxxxx/
> Closes: https://lore.kernel.org/all/20260909115723.528501-1-trintaeoitogc@xxxxxxxxx/
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> ---
> v2:
> - Renamed vma_is_faulted() to vma_anon_tracked() as per David.
> 
> v1:
> https://lore.kernel.org/r/20260909-vma-is-faulted-v1-1-3a701f48984a@xxxxxxxxxx
> ---
>  include/linux/mmap_lock.h         | 12 ++++++------
>  mm/huge_memory.c                  |  4 ++--
>  mm/internal.h                     |  2 +-
>  mm/khugepaged.c                   |  3 +++
>  mm/ksm.c                          | 10 +++++-----
>  mm/madvise.c                      |  4 ++--
>  mm/memory.c                       |  2 +-
>  mm/mprotect.c                     |  2 +-
>  mm/mremap.c                       |  4 ++--
>  mm/rmap.c                         | 22 +++++++++++-----------
>  mm/swapfile.c                     |  2 +-
>  mm/userfaultfd.c                  |  2 +-
>  mm/vma.c                          | 29 +++++++++++++++--------------
>  mm/vma.h                          | 22 +++++++++++++++++++++-
>  tools/testing/vma/include/dup.h   |  2 +-
>  tools/testing/vma/include/stubs.h |  4 ++++
>  16 files changed, 77 insertions(+), 49 deletions(-)
> 
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 00eae65b74bd..03e1eb136111 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -273,7 +273,7 @@ static inline void vma_end_read(struct vm_area_struct *vma)
>  	vma_refcount_put(vma);
>  }
>  
> -static inline unsigned int __vma_raw_mm_seqnum(struct vm_area_struct *vma)
> +static inline unsigned int __vma_raw_mm_seqnum(const struct vm_area_struct *vma)
>  {
>  	const struct mm_struct *mm = vma->vm_mm;
>  
> @@ -288,7 +288,7 @@ static inline unsigned int __vma_raw_mm_seqnum(struct vm_area_struct *vma)
>   *
>   * Returns true if write-locked, otherwise false.
>   */
> -static inline bool __is_vma_write_locked(struct vm_area_struct *vma)
> +static inline bool __is_vma_write_locked(const struct vm_area_struct *vma)
>  {
>  	/*
>  	 * current task is holding mmap_write_lock, both vma->vm_lock_seq and
> @@ -344,7 +344,7 @@ int vma_start_write_killable(struct vm_area_struct *vma)
>   * vma_assert_write_locked() - assert that @vma holds a VMA write lock.
>   * @vma: The VMA to assert.
>   */
> -static inline void vma_assert_write_locked(struct vm_area_struct *vma)
> +static inline void vma_assert_write_locked(const struct vm_area_struct *vma)
>  {
>  	if (!IS_ENABLED(CONFIG_MMU)) {
>  		mmap_assert_write_locked(vma->vm_mm);
> @@ -359,7 +359,7 @@ static inline void vma_assert_write_locked(struct vm_area_struct *vma)
>   * lock and is not detached.
>   * @vma: The VMA to assert.
>   */
> -static inline void vma_assert_locked(struct vm_area_struct *vma)
> +static inline void vma_assert_locked(const struct vm_area_struct *vma)
>  {
>  	unsigned int refcnt;
>  
> @@ -410,7 +410,7 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
>   * With lockdep disabled we may sometimes race with other threads acquiring the
>   * mmap read lock simultaneous with our VMA read lock.
>   */
> -static inline void vma_assert_stabilised(struct vm_area_struct *vma)
> +static inline void vma_assert_stabilised(const struct vm_area_struct *vma)
>  {
>  	/*
>  	 * If another thread owns an mmap lock, it may go away at any time, and
> @@ -445,7 +445,7 @@ static inline void vma_assert_stabilised(struct vm_area_struct *vma)
>  	vma_assert_locked(vma);
>  }
>  
> -static inline bool vma_is_attached(struct vm_area_struct *vma)
> +static inline bool vma_is_attached(const struct vm_area_struct *vma)
>  {
>  	return refcount_read(&vma->vm_refcnt);
>  }

nit: I would suggest you do all of this constification in a separate patch,
otherwise it can become tricky for backports. Things can naturally start depending
on const correctness, and you either need to manually adjust those future patches,
or try to backport this one.

The rest looks vaguely good to me. I admit I'm not a gigantic fan of the name,
but I suspect it fits better with your future anon rmap ideas :)

Reviewed-by: Pedro Falcato <pfalcato@xxxxxxx>


-- 
Pedro