Re: [PATCH 01/15] mm/rmap: introduce anon_rmap APIs for anonymous folios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Lockless reads of anon_vma->root in folio_maybe_same_anon_vma() lack READ_ONCE(), causing data races.
- [High] The anon_rmap_t API lacks a NULL validation helper and its cleanup/lock functions crash on ANON_RMAP_NULL.
- [Medium] vma_get_anon_rmap() incorrectly uses mmap_assert_locked(vma->vm_mm) instead of vma_assert_locked(vma).
- [Medium] The anon_rmap_foreach_vma macro evaluates the anon_rmap argument multiple times, creating an API hazard.
--

commit d698495fb94a40d2764331cdf26a351a9f9a97a3
Author: tao <tao.wangtao@honor.com>

mm/rmap: introduce anon_rmap APIs for anonymous folios
    
This patch adds a set of anon_rmap APIs to operate on the reverse mappings
of anonymous folios. It introduces anon_rmap_for_each_vma() as a wrapper
around the interval tree to prepare for RCU-based lockless rmap traversal.

> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 8dc0871e5f001..c42314ea43622 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
[ ... ]
> @@ -969,6 +1007,36 @@ void rmap_walk_locked(struct folio *folio, struct rmap_walk_control *rwc);
>  struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  					  struct rmap_walk_control *rwc);
>  
> +bool folio_maybe_same_anon_vma(const struct folio *folio,
> +	const struct vm_area_struct *vma);
> +anon_rmap_t folio_get_anon_rmap(const struct folio *folio);
> +anon_rmap_t folio_lock_anon_rmap_read(const struct folio *folio,
> +	struct rmap_walk_control *rwc);
> +
> +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);
> +
> +	*avc = anon_vma_interval_tree_iter_first(&anon_vma->rb_root, start, last);
> +	return *avc ? (*avc)->vma : NULL;
> +}
> +
> +static inline struct vm_area_struct *anon_rmap_iter_next_vma(
> +	anon_rmap_t anon_rmap, unsigned long start, unsigned long last,
> +	struct anon_vma_chain **avc)
> +{
> +	if (!*avc)
> +		return NULL;
> +	*avc = anon_vma_interval_tree_iter_next(*avc, start, last);
> +	return *avc ? (*avc)->vma : NULL;
> +}
> +
> +#define anon_rmap_foreach_vma(vma, avc, anon_rmap, start, last) 	   \
> +	for (vma = anon_rmap_iter_first_vma(anon_rmap, start, last, &avc); \
> +	     vma; vma = anon_rmap_iter_next_vma(anon_rmap, start, last, &avc))

[Severity: Medium]
Can this macro cause unintended side effects if a caller passes an expression
like folio_get_anon_rmap(folio) as the anon_rmap argument?

It appears the argument is evaluated on every single loop iteration in the
update step, which could result in repeatedly executing any side effects.

Should the anon_rmap parameter be removed from the anon_rmap_iter_next_vma
call, as it seems to be unused there?

> +
>  #else	/* !CONFIG_MMU */
>  
>  #define anon_vma_init()		do {} while (0)
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 99e1b3dc390b7..bbfd08ffae3db 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -701,6 +701,79 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	return anon_vma;
>  }
>  
> +anon_rmap_t vma_get_anon_rmap(struct vm_area_struct *vma)
> +{
> +	mmap_assert_locked(vma->vm_mm);

[Severity: Medium]
Will this trigger a lockdep warning or BUG if reached from a per-VMA locked
path?

Since this API is intended for generic anonymous rmap operations, should it
use vma_assert_locked(vma) instead to properly support per-VMA locking?

> +	VM_BUG_ON(!vma->anon_vma);
> +	get_anon_vma(vma->anon_vma);
> +	return anon_vma_to_anon_rmap(vma->anon_vma);
> +}
> +
> +void put_anon_rmap(anon_rmap_t anon_rmap)
> +{
> +	put_anon_vma(anon_rmap_to_anon_vma(anon_rmap));
> +}

[Severity: High]
What happens if a caller passes ANON_RMAP_NULL to this function, or to the
locking functions like anon_rmap_lock_write()?

Since ANON_RMAP_NULL resolves to a NULL pointer, passing it to put_anon_vma()
or anon_vma_lock_write() would immediately trigger a NULL pointer dereference.

Because anon_rmap_t is a struct, standard C idioms like if (!rmap) cannot be
used to check for errors.

Would it make sense to provide an anon_rmap_valid() helper and handle NULL
handles explicitly in these paths?

[ ... ]
> +void anon_rmap_unlock_read(anon_rmap_t anon_rmap)
> +{
> +	anon_vma_unlock_read(anon_rmap_to_anon_vma(anon_rmap));
> +}
> +
> +bool folio_maybe_same_anon_vma(const struct folio *folio,
> +	const struct vm_area_struct *vma)
> +{
> +	struct anon_vma *anon_vma;
> +	struct anon_vma *tgt_anon_vma = vma->anon_vma;
> +	bool same = false;
> +
> +	rcu_read_lock();
> +	anon_vma = folio_anon_vma(folio);
> +	if (anon_vma && tgt_anon_vma)
> +		same = anon_vma->root == tgt_anon_vma->root;

[Severity: High]
Does this lockless read of anon_vma->root need to use READ_ONCE()?

Because anon_vma = folio_anon_vma(folio) is called under rcu_read_lock()
without incrementing the refcount, and anon_vma objects are allocated with
SLAB_TYPESAFE_BY_RCU, could the object be freed and recycled?

A plain C read of anon_vma->root might race with concurrent initializations of
the recycled object.

> +	rcu_read_unlock();
> +	return same;
> +}

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