Re: [PATCH 08/15] mm: prepare rmap infrastructure for ANON_VMA_LAZY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Race condition and missing synchronization in `anon_vma_lazy_lock_read()` and `anon_vma_tree_lock_write()` for lazy VMAs.
- [Critical] Ignored return value of `vma_get()` in `anon_vma_lazy_lock_read()` leads to Use-After-Free and refcount underflow.
- [High] Off-by-one boundary check in `anon_rmap_iter_first_vma()` causes false positive VMA overlap detection and out-of-bounds page table accesses.
--

commit 411934596db1ffd0b45c2469ce0c285daaf6e6e1
Author: tao <tao.wangtao@honor.com>

mm: prepare rmap infrastructure for ANON_VMA_LAZY

Introduce ANON_VMA_LAZY helpers and prepare the anon_rmap and anon_vma_tree
infrastructure for the upcoming ANON_VMA_LAZY feature.

> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 9802bce926959..ebe9f3f61170d 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h

[ ... ]

> @@ -1015,8 +1047,17 @@ static inline struct vm_area_struct *anon_rmap_iter_first_vma(
>  	anon_rmap_t anon_rmap, unsigned long start, unsigned long last,
>  	struct anon_vma_chain **avc)
>  {
> -	struct anon_vma *anon_vma = anon_rmap_to_anon_vma(anon_rmap);
> +	struct anon_vma *anon_vma;
> +
> +	*avc = NULL;
> +	if (!anon_rmap_is_anon_vma(anon_rmap)) {
> +		struct vm_area_struct *vma = anon_rmap_to_vma(anon_rmap);
>  
> +		if (vma->vm_pgoff + vma_pages(vma) < start || vma->vm_pgoff > last)
                                                    ^^^^^^

[Severity: High]
Does this off-by-one boundary check cause false positive VMA overlap detection?

It looks like the exclusive end check uses < start instead of <= start.
If a folio's start offset is exactly at the VMA's exclusive end, this check
evaluates to false, incorrectly implying the folio overlaps with the VMA.

The iteration would then yield the non-overlapping VMA to rmap_walk_anon(),
which passes the offset to vma_address().

Because the offset is outside the VMA bounds, vma_address() calculates an
address >= vma->vm_end and returns -EFAULT. This triggers
VM_BUG_ON_VMA(address == -EFAULT, vma) in rmap_walk_anon().

> +			return NULL; /* No overlap in the VMA range. */
> +		return vma;
> +	} else
> +		anon_vma = anon_rmap_to_anon_vma(anon_rmap);
>  	*avc = anon_vma_interval_tree_iter_first(&anon_vma->rb_root, start, last);
>  	return *avc ? (*avc)->vma : NULL;
>  }

[ ... ]

> diff --git a/mm/internal.h b/mm/internal.h
> index 639f9c287f4c2..6b703646f66db 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h

[ ... ]

> +/*
> + * ANON_VMA_TREE_VMA is just a VMA, without anon_vma or anon_vma_chain,
> + * so no protection is needed.
> + */
>  static inline void anon_vma_tree_lock_write(anon_vma_tree_t anon_tree)
>  {
>  	struct anon_vma *anon_vma = anon_vma_tree_anon_vma(anon_tree);
>  
> -	anon_vma_lock_write(anon_vma);
> +	if (anon_vma)
> +		anon_vma_lock_write(anon_vma);

[Severity: Critical]
Can this missing synchronization lead to a use-after-free of page table pages?

Here, anon_vma_tree_lock_write() acts as a no-op for lazy VMAs.
In mm/rmap.c, anon_vma_lazy_lock_read() only increments vm_rcuref via vma_get()
without acquiring any synchronization locks.

Consequently, operations that traverse page tables (like rmap_walk_anon()) run
completely lockless and concurrently with page table structural changes like
vma_adjust_trans_huge() in __split_vma() or VMA teardowns in free_pgtables().

vma_get() doesn't prevent free_pgd_range() from unmapping and freeing the page
tables. This allows page tables to be freed while page_vma_mapped_walk() is
traversing them, which can lead to a use-after-free of page table pages.

>  }
>  
>  static inline int anon_vma_tree_trylock_write(anon_vma_tree_t anon_tree)

[ ... ]

> diff --git a/mm/rmap.c b/mm/rmap.c
> index 58f470e0c3ffd..d13395a082c60 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c

[ ... ]

> +static void anon_vma_lazy_lock_read(struct vm_area_struct *vma)
> +{
> +	vma_get(vma);

[Severity: Critical]
Is it safe to ignore the return value of vma_get() here?

vma_get() returns NULL if the VMA's reference count is already zero, indicating
it is scheduled for freeing.

Because this failure is ignored and the function returns void, callers like
anon_rmap_lock_read() might proceed to traverse a dying or freed VMA, assuming
the lock and reference were successfully acquired.

This can lead to a use-after-free when accessing VMA fields.

Additionally, anon_vma_lazy_unlock_read() will later call vma_put(), which
invokes rcuref_put() on an already-zero refcount, causing a refcount underflow
and potential double-free of the VMA memory.

> +}
> +
> +static bool anon_vma_lazy_trylock_read(struct vm_area_struct *vma)
> +{
> +	return (bool)vma_get(vma);
> +}
> +
> +static void anon_vma_lazy_unlock_read(struct vm_area_struct *vma)
> +{
> +	vma_put(vma);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527110147.17815-1-tao.wangtao@honor.com?part=8