Re: [RFC PATCH v2 10/13] mm: replace anon_vma with anon_node for ANON_VMA_FRACTAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Desynchronization of `prev` and `tmp_prev` pointers in `dup_mmap()` leads to `anon_node` tree corruption when VMAs are skipped, causing page use-after-free and structural corruption.
- [Critical] The `ANON_RMAP_FOREACH_VMA` macro enters an infinite loop when callers use a `continue` statement, leading to a hard lockup.
- [High] Stubbing `anon_vma_lock_write` to a no-op breaks locking in `mremap` and `khugepaged`, leading to concurrent page table corruption.
--

--- Patch [10]: [RFC PATCH v2 10/13] mm: replace anon_vma with anon_node for ANON_VMA_FRACTAL ---
commit 1deef9c4b5a4ec2adb2fbed6a3ce6762667d351c
Author: tao <tao.wangtao@honor.com>
Subject: mm: replace anon_vma with anon_node for ANON_VMA_FRACTAL

This commit replaces anon_vma with anon_node for anonymous reverse mapping
walks, aiming to save memory by removing anon_vma_chain from VMA. It
introduces a new ANON_RMAP_FOREACH_VMA macro and stubs out older locking
APIs.

> diff --git a/mm/internal.h b/mm/internal.h
> index 1c29f98b1b252..7c34ff1abf85c 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
[ ... ]
> +/* Huge pages are protected by folio_lock(), these APIs no longer needed. */
> +static inline void anon_vma_lock_write(struct anon_vma *anon_vma) {}
> +static inline int anon_vma_trylock_write(struct anon_vma *anon_vma) { return 1; }
> +static inline void anon_vma_unlock_write(struct anon_vma *anon_vma) {}

[Severity: High]
Does stubbing out anon_vma_lock_write() leave page table modifications
unprotected against concurrent reverse mapping accesses?

Operations like take_rmap_locks() in mm/mremap.c and collapse_huge_page()
in mm/khugepaged.c still rely on this lock to serialize page table changes.
Without it, there could be page table corruption when try_to_unmap()
locklessly scans them.

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 2311ae7c2ff45..c283e0de819af 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
[ ... ]
>  	mt_clear_in_rcu(vmi.mas.tree);
> -	for_each_vma(vmi, mpnt) {
> +	for ( ; (mpnt = vma_next(&vmi)) != NULL; prev = mpnt, tmp_prev = tmp) {
>  		struct file *file;

[Severity: Critical]
Does this loop advancement logic cause prev and tmp_prev to become
desynchronized if a VMA is skipped?

If mpnt->vm_flags & VM_DONTCOPY is true, the loop in dup_mmap() executes a
continue statement. The step expression will update prev to mpnt (the skipped
VMA), but tmp_prev will remain pointing to the clone of an unrelated older
VMA.

[ ... ]
>  			tmp->anon_vma = NULL;
> -		} else if (anon_vma_fork(tmp, mpnt))
> +		} else if (anon_vma_fork_with_prev(tmp, mpnt, tmp_prev, prev))
>  			goto fail_nomem_anon_vma_fork;

[Severity: Critical]
If prev and tmp_prev are desynchronized due to a prior skipped VMA, will
this cause anon_vma_fork_with_prev() to incorrectly reuse an anon_node from
an unrelated mapping?

This could place child VMAs in the wrong reverse mapping tree, leading to
use-after-free regressions when the kernel attempts to unmap or swap out
pages.

> diff --git a/mm/rmap.c b/mm/rmap.c
> index aa6632790d9af..d3677515fae92 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
[ ... ]
> @@ -3455,9 +3460,7 @@ static void rmap_walk_anon(struct folio *folio,
>  
>  	pgoff_start = folio_pgoff(folio);
>  	pgoff_end = pgoff_start + folio_nr_pages(folio) - 1;
> -	anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root,
> -			pgoff_start, pgoff_end) {
> -		struct vm_area_struct *vma = avc->vma;
> +	ANON_RMAP_FOREACH_VMA(anon_vma, 0, pgoff_start, pgoff_end, ({
>  		unsigned long address = vma_address(vma, pgoff_start,
>  				folio_nr_pages(folio));
>  

[Severity: Critical]
Can the use of continue inside this macro block bypass the iterator
advancement logic of ANON_RMAP_FOREACH_VMA?

When CONFIG_ANON_VMA_FRACTAL is enabled, ANON_RMAP_FOREACH_VMA is defined
as a while loop that advances the iterator at the end of the block. If a
caller like rmap_walk_anon() jumps to the next iteration using continue,
it will skip the advancement and result in an infinite loop and softlockup.

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