Re: [RFC PATCH v2 07/13] mm: handle rmap_base changes for ANON_VMA_FRACTAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Self-deadlock in VMA modification paths due to recursive acquisition of the anon_vma rw_semaphore, followed by unbalanced unlock leading to semaphore corruption.
--

--- Patch [7]: [RFC PATCH v2 07/13] mm: handle rmap_base changes for ANON_VMA_FRACTAL ---
commit 4ece2a0d48ed77392c6b5765afc8e465e5eeb821
Author: tao <tao.wangtao@honor.com>

mm: handle rmap_base changes for ANON_VMA_FRACTAL

This patch introduces vma_pre_update_rmap_base() and
vma_post_update_rmap_base() to track rmap_base changes during VMA merges
and shrinks. It calls these helpers before vma_prepare() and after
vma_complete() in paths where the rmap_base may change.

> diff --git a/mm/vma.c b/mm/vma.c
> index 9eea2850818a8..d2996369e30d6 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
[ ... ]
> @@ -748,12 +749,16 @@ static int commit_merge(struct vma_merge_struct *vmg)
>  	/*
>  	 * If vmg->give_up_on_oom is set, we're safe, because we don't actually
>  	 * manipulate any VMAs until we succeed at preallocation.
>  	 */
>  	if (vma_iter_prealloc(vmg->vmi, vma))
>  		return -ENOMEM;
>  
> +	rbase_diff = rmap_base(vmg->start, vmg->pgoff) - vma_rmap_base(vma);
> +	if (rbase_diff && vma_pre_update_rmap_base(vma, rbase_diff)) {
> +		vma_iter_free(vmg->vmi);
> +		return -ENOMEM;
> +	}
> +
>  	vma_prepare(&vp);

[Severity: High]
Will this sequence cause a self-deadlock on the anon_vma rw_semaphore?

Looking at vma_pre_update_rmap_base(), it acquires the write lock on the
semaphore via down_write(anon_node_rmap_sem(anon_nod)). 

Immediately after that, vma_prepare() is called here. Since vma_prepare()
unconditionally calls anon_vma_lock_write() which attempts to acquire the
exact same rw_semaphore, won't the second down_write() block indefinitely
because Linux rw_semaphores are not recursive for writers?

The exact same pattern is also present in vma_shrink().

> diff --git a/mm/rmap.c b/mm/rmap.c
> index 900a9d9e5e6a3..2a31f0441c114 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
[ ... ]
> +void vma_post_update_rmap_base(struct vm_area_struct *vma, unsigned long diff)
> +{
> +	struct anon_node *anon_nod = vma_anon_node(vma);
> +	struct rw_semaphore *rmap_sem;
> +	struct anon_node *release_node = NULL;
> +
> +	vma_assert_write_locked(vma);
> +	if (!anon_nod || !diff)
> +		return;
> +
> +	rmap_sem = anon_node_rmap_sem(anon_nod);
> +	rwsem_assert_held_write(rmap_sem);
> +	anon_node_untrack_rmap_locked(anon_nod, vma,
> +		vma_rmap_base(vma) - diff, &release_node);
> +	up_write(rmap_sem);

[Severity: High]
Does this up_write() corrupt the semaphore state?

This function is called in commit_merge() and vma_shrink() right after
vma_complete(). Because vma_complete() releases the locks previously
acquired by vma_prepare(), the rw_semaphore has already been unlocked by the
time we reach this code.

Will calling up_write() on an already-unlocked semaphore cause kernel
instability or further lock corruption?

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