Re: [RFC PATCH v2 11/13] mm: optimize rmap for ANON_VMA_FRACTAL with PVL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Use-After-Free of `mm_struct` in `vma_refcount_put()` due to dropped `mm` pin.
- [High] Per-VMA lock `vm_lock_seq` exclusion bypass in `trylock_leaf_vma_rmap()`.
- [High] Lockless speculative access of `anon_nod->mm` risks dereferencing a freed pointer.
- [High] Use-After-Free of `anon_vma` on `folio_lock_anon_vma_read()` error path.
- [High] `trylock_leaf_vma_rmap()` locks completely unrelated VMAs due to missing `anon_vma` validation.
- [High] TOCTOU race due to missing VMA bounds re-validation after acquiring the VMA read lock.
- [High] Valid zero-address VMAs silently fail the fast path and are ignored by rmap iteration.
--

--- Patch [11]: [RFC PATCH v2 11/13] mm: optimize rmap for ANON_VMA_FRACTAL with PVL ---
commit e9e971f63165144548b2891ce74a7cd4448a1490
Author: tao <tao.wangtao@honor.com>

mm: optimize rmap for ANON_VMA_FRACTAL with PVL
    
Leaf anon_nodes have no children, and a given address can belong to at
most one VMA. In this case, PVL can protect the rmap.

> diff --git a/mm/rmap.c b/mm/rmap.c
> index d3677515fae92..0967ba2d2a466 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c

[ ... ]

> @@ -1035,9 +1037,48 @@ void anon_node_lock_rmap(struct anon_node *anon_nod)
>  
>  void anon_node_unlock_rmap(struct anon_node *anon_nod)
>  {
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/* Corresponds to folio_lock_anon_vma_read(). */
> +	if (anon_rmap_is_leaf_vma(anon_nod)) {
> +		vma_refcount_put(anon_rmap_to_leaf_vma(anon_nod));

[Severity: High]
Can this cause a use-after-free crash by accessing an unpinned mm_struct?

In trylock_leaf_vma_rmap(), the original mm_struct is temporarily pinned
via mmget_not_zero(mm), but the pin is immediately dropped with
mmput_async(mm). It then checks vma->vm_mm == mm and sets other_mm = NULL.

The caller folio_lock_anon_vma_read() sees other_mm == NULL and returns
the tagged leaf VMA without holding any pin (via mmgrab()) on the
mm_struct. When the rmap operation completes, anon_node_unlock_rmap()
calls vma_refcount_put() here. Since the thread holds no reference to
the mm_struct, it can be freed concurrently. vma_refcount_put() then
accesses the unpinned mm_struct to call
rcuwait_wake_up(&vma->vm_mm->vma_writer_wait), which can crash.

> +		return;
> +	}
> +#endif
> +
>  	up_read(anon_node_rmap_sem(anon_nod));
>  }
>  
> +static struct vm_area_struct *trylock_leaf_vma_rmap(
> +		struct anon_node *anon_nod, const struct folio *folio,
> +		struct mm_struct **other_mm)
> +{
> +	unsigned long pgoff_start = folio_pgoff(folio);
> +	unsigned long pgoff_end = pgoff_start + folio_nr_pages(folio) - 1;
> +	unsigned long addr = anon_node_rmap_address(anon_nod, pgoff_start);
> +	struct mm_struct *mm = anon_nod->mm;
> +	struct vm_area_struct *vma;
> +	int oldcnt;
> +
> +	if (anon_nod->nr_children)
> +		return NULL;
> +
> +	if (!addr || !mm || !mmget_not_zero(mm))

[Severity: High]
Is it safe to read anon_nod->mm locklessly under RCU?

The anon_node structure is allocated with SLAB_TYPESAFE_BY_RCU, meaning
the object itself can be freed and reused. However, the mm_struct it
points to is not RCU protected and can be completely freed and returned
to the allocator. Dereferencing the potentially stale mm pointer from
the SLAB-recycled anon_nod and passing it to mmget_not_zero() could
access unmapped memory or corrupt unrelated structures.

[Severity: High]
Does this unintentionally exclude valid VMAs mapped at address zero?

anon_node_rmap_address() can legitimately return 0 for an anonymous VMA
mapped at virtual address 0. Checking if (!addr) treats 0 as an error
rather than a valid address, which silently skips valid zero-address
VMAs for rmap walkers, hwpoison, and KSM.

> +		return NULL;
> +
> +	vma = vma_lookup(mm, addr);
> +	mmput_async(mm);
> +	if (!vma || vma_last_pgoff(vma) < pgoff_end)

[Severity: High]
Could this inadvertently lock a completely unrelated VMA?

This uses vma_lookup() to locate a VMA and returns it if it covers the
target offset, but fails to verify that the returned VMA's anon_vma
pointer matches the anon_nod, or even if the VMA is anonymous. If the
original VMA was unmapped and a completely unrelated VMA was mapped at
the exact same address, vma_lookup() will find it. This unrelated VMA
will be incorrectly locked and returned to the rmap walker, skipping
the true anon_vma tree and executing callbacks on the wrong VMA.

> +		return NULL;
> +
> +	if (!__refcount_inc_not_zero_limited_acquire(&vma->vm_refcnt, &oldcnt,
> +						     VM_REFCNT_LIMIT)) {

[Severity: High]
Does this bypass the required per-VMA lock vm_lock_seq exclusion?

The per-VMA lock algorithm strictly requires a reader to increment
vm_refcnt and then verify that vma->vm_lock_seq != mm->mm_lock_seq to
ensure no writer is currently locking the VMA. This successfully grants
the VMA read lock purely by incrementing vm_refcnt, failing to perform
any vm_lock_seq exclusion checks. A reader will incorrectly proceed even
if a writer holds the VMA lock, leading to potential data corruption as
the rmap walker accesses page tables concurrently with structural VMA
modifications.

[Severity: High]
Is there a time-of-check to time-of-use race here?

vma_last_pgoff(vma) < pgoff_end is checked locklessly under RCU just
before this. Between that check and the increment of vm_refcnt to
acquire the read lock, a concurrent writer could modify the VMA. Because
this function fails to re-check the VMA bounds after locking, the rmap
walker might receive a locked VMA that is too small and no longer covers
the required address range, resulting in out-of-bounds page table
accesses.

> +		return NULL;
> +	}
> +
> +	*other_mm = (vma->vm_mm == mm) ? NULL : vma->vm_mm;
> +	return vma;
> +}

[ ... ]

> @@ -1156,9 +1208,8 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	}
>  
>  	if (!folio_mapped(folio)) {
> -		rcu_read_unlock();
>  		put_anon_vma(anon_vma);
> -		return NULL;
> +		goto out;

[Severity: High]
Will this leave the caller with a freed anon_vma pointer?

In this fallback path, if down_read_trylock() fails and
!folio_mapped(folio) is true, the code calls put_anon_vma(anon_vma).
The original code correctly returned NULL at this point. By replacing
this with goto out, it falls through to return anon_vma without setting
anon_vma to NULL. The caller receives a non-NULL pointer, falsely
assuming the read lock was acquired, and will operate on the unpinned
and potentially freed anon_vma object.

>  	}
>  
>  	/* we pinned the anon_vma, its safe to sleep */

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