Re: [RFC PATCH v2 04/13] mm: implement anon_node_clone for ANON_VMA_FRACTAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Flattening the fractal tree into a single list corrupts child iteration when children have their own descendants, permanently severing the tree structure.
- [Critical] VMA unlinking unconditionally casts anon_node to anon_vma and overwrites mismatched fields.
- [High] Unsynchronized read-modify-write of root->depth causes data races and structural corruption of the fractal tree.
- [High] Newly allocated anon_node objects are permanently leaked because their reference counts are never decremented during VMA teardown.
- [High] anon_node objects are never removed from fractal_list, leading to a Kernel BUG and Use-After-Free.
- [High] Intermediate anon_node structures can be freed while they still have children in their fractal_list, leading to a VM_BUG_ON panic or Use-After-Free.
- [High] anon_node slab objects allocated in anon_node_track_rmap() with is_fork=false are permanently leaked because ownership is never transferred to a VMA.
- [Medium] Missing state re-validation after lock upgrade in anon_node_track_rmap() leads to duplicate child node insertions.
--

--- Patch [4]: [RFC PATCH v2 04/13] mm: implement anon_node_clone for ANON_VMA_FRACTAL ---
commit d6053627649639aa9d6f587172befdbc0e783aed
Author: tao <tao.wangtao@honor.com>

mm: implement anon_node_clone for ANON_VMA_FRACTAL

This commit implements anon_node_clone for ANON_VMA_FRACTAL. It creates
a child anon_node when the VMA is remapped or when the VMA count exceeds
the maximum trackable limit. Newly created nodes are inserted at the tail
of the rbc children list.

> diff --git a/mm/rmap.c b/mm/rmap.c
> index 55074ee931967..9dc82e7438b00 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c

[ ... ]

> +static inline void vma_set_anon_node(struct vm_area_struct *vma,
> +		struct anon_node *anon_nod, bool is_locked)
> +{
> +	struct rw_semaphore *rmap_sem = anon_node_rmap_sem(anon_nod);
> +
> +	if (is_locked) {
> +		vma->anon_vma = (struct anon_vma *)anon_nod;
> +		return;
> +	}
> +	down_write(rmap_sem);
> +	vma->anon_vma = (struct anon_vma *)anon_nod;

[Severity: Critical]
Does this type cast corrupt the anon_node structure during teardown?

During teardown, the standard unlink_anon_vmas() flow operates on this pointer
and executes active_anon_vma->num_active_vmas--. Since anon_node has a
different layout and lacks num_active_vmas, won't this unconditionally write
out of bounds and corrupt the anon_node fields?

> +	up_write(rmap_sem);
> +}
> +
> +static int anon_node_add_child(struct anon_node *anon_nod,
> +	struct vm_area_struct *vma, bool is_fork)
> +{
> +	struct anon_node *child;
> +	struct anon_node *root = anon_nod->root;
> +	struct rw_semaphore *rmap_sem = anon_node_rmap_sem(root);
> +	struct anon_node *rbc_ch = anon_nod, *next;
> +
> +	if (root->depth == 0)
> +		root->depth = 1;

[Severity: High]
Can this unsynchronized read-modify-write cause data races?

root->depth is being modified outside of any locks. Could a concurrent thread
allocating a node via anon_node_alloc() read an incorrect parent->depth of 0
instead of 1? Because tree traversals require accurate depths, won't an
incorrectly initialized branch permanently break the tree structure?

> +	child = anon_node_alloc(vma, anon_nod, is_fork);

[Severity: High]
Are these newly allocated anon_node objects permanently leaked?

anon_node is allocated with a refcount of 1. During VMA teardown,
unlink_anon_vmas() relies on iterating vma->anon_vma_chain to call
put_anon_vma(). However, since anon_node logic never populates anon_vma_chain,
won't the iteration loop silently skip over it and leak the memory?

[Severity: High]
Does anon_node_alloc() fail to pin the direct parent node?

In anon_node_alloc(), the logic increments the refcount of the root node but
doesn't increment the refcount of the direct parent. If an intermediate parent
node is unmapped, won't its refcount drop to 0 and get freed while it still
has children in its fractal_list? This could lead to a VM_BUG_ON panic or a
use-after-free for the children.

> +	if (!child)
> +		return -ENOMEM;
> +
> +	rmap_sem = anon_node_rmap_sem(root);
> +	down_write(rmap_sem);
> +	/* Add new child to the tail of rbc children. */
> +	while ((next = anon_node_next_rbc_child(anon_nod, rbc_ch)) != NULL) {
> +		rbc_ch = next;
> +	}
> +	list_add(&child->fractal_list, &rbc_ch->fractal_list);

[Severity: Critical]
Does inserting the new child here corrupt the contiguous list iteration?

The iterator macro anon_node_next_rbc_child() strictly expects contiguous
children and returns NULL when node->depth != parent->depth + 1:

mm/internal.h:anon_node_next_rbc_child() {
    ...
    return node->depth == anon_nod->depth + 1 ? node : NULL;
}

If an existing child has its own descendants, those descendants interrupt the
contiguous sequence. The loop above stops prematurely at the first child with
descendants. By inserting the new child via list_add() immediately after that
child, won't the new sibling be placed before the descendants? This would
break the depth check for both anon_node_next_rbc_child() and
anon_node_next_descendant(), permanently severing all subsequent subtrees.

[Severity: High]
Are anon_node objects ever removed from fractal_list during teardown?

Nodes are added to the list here, but it doesn't appear there is a
corresponding list_del() anywhere in the teardown logic. If anon_node_free()
were reached, wouldn't it assert VM_BUG_ON(!list_empty(&anon_nod->fractal_list))?
Furthermore, freeing a node without removing it from the list would leave
dangling list pointers in its parent and siblings, causing a use-after-free
on the next tree traversal.

> +	++anon_nod->nr_children;
> +
> +	if (is_fork)
> +		vma_set_anon_node(vma, child, true);

[Severity: High]
Are anon_node objects allocated with is_fork=false permanently leaked?

When anon_node_track_rmap() expands tracking by calling anon_node_add_child()
with is_fork=false, the vma_set_anon_node() call here is skipped. Because the
newly allocated child node is never attached to a VMA, is there any mechanism
to track or decrement its initial reference count when the VMA is ultimately
unmapped?

> +	up_write(rmap_sem);
> +	return 0;
> +}
> +
> +/* Returns 0 on success, non-zero on failure. */
> +static int anon_node_track_rmap(struct anon_node *anon_nod,
> +		struct vm_area_struct *vma)
> +{
> +	struct rw_semaphore *rmap_sem = anon_node_rmap_sem(anon_nod);
> +	unsigned long rmap_base = vma_rmap_base(vma);
> +	struct anon_node *rbc_ch = anon_nod;
> +
> +	if (try_track_rmap(anon_nod, rmap_base))
> +		return 0;
> +
> +	down_read(rmap_sem);
> +	while ((rbc_ch = anon_node_next_rbc_child(anon_nod, rbc_ch)) != NULL) {
> +		if (try_track_rmap(rbc_ch, rmap_base))
> +			break;
> +	}
> +	up_read(rmap_sem);
> +	if (rbc_ch)
> +		return 0;
> +
> +	return anon_node_add_child(anon_nod, vma, false);

[Severity: Medium]
Should the state be re-validated after acquiring the write lock?

anon_node_track_rmap() searches for a capable child node under a read lock.
If it fails, it drops the read lock and calls anon_node_add_child(), which
takes a write lock to unconditionally allocate and append a node.

Could concurrent threads that fail the read check queue up on the write lock
and blindly append redundant tracking nodes for the exact same rmap_base?

> +}

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