Re: [REGRESSION] mremap() fails VMA expansion when VM_MIXEDMAP is set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: vma_merge_new_range() merges VM_MIXEDMAP VMAs in mmap() and mremap()
- [Medium] mm: vma: can_merge_vma() breaks test_vma_merge_special_flags() for VM_MIXEDMAP
--

commit b9b12caabde38874f12fe6aa692b1dcf956d3f4a
Author: Luiz Capitulino <luizcap@redhat.com>

mremap() fails VMA expansion when VM_MIXEDMAP is set

This patch allows vma_merge_new_range() to merge VM_MIXEDMAP VMAs while
updating can_merge_remove_vma() to reject removing VM_MIXEDMAP VMAs so
that in-place VMA expansion during mremap() can succeed.

> diff --git a/mm/vma.c b/mm/vma.c
> index f56317ee82471..12ba8e247b477 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -851,9 +851,15 @@ static int commit_merge(struct vma_merge_struct *vmg)
>  	return 0;
>  }
>  
> -/* We can only remove VMAs when merging if they do not have a close hook. */
>  static bool can_merge_remove_vma(struct vm_area_struct *vma)
>  {
> +	if (vma_flags_test(&vma->flags, VMA_MIXEDMAP_BIT))
> +		return false;
> +
> +	/*
> +	 * We can only remove VMAs when merging if they do not have a
> +	 * close hook.
> +	 */
>  	return !vma->vm_ops || !vma->vm_ops->close;
>  }
>  
> @@ -1093,6 +1099,20 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
>  	return NULL;
>  }
>  
> +static bool can_merge_vma(const struct vma_merge_struct *vmg)
> +{
> +	/* VMAs without prev/next are unmergeable */
> +	if (!vmg->prev && !vmg->next)
> +		return false;
> +
> +	/* We can't merge VMA_IO, VMA_DONTEXPAND or VMA_PFNMAP */
> +	if (vma_flags_test_any(&vmg->vma_flags, VMA_IO_BIT, VMA_DONTEXPAND_BIT,
> +			       VMA_PFNMAP_BIT))
> +		return false;
> +
> +	return true;
> +}

[Severity: High]
Does omitting VMA_MIXEDMAP_BIT from can_merge_vma() cause unintended merges
of distinct VM_MIXEDMAP VMAs during mmap() and mremap() moves?

In vma_merge_new_range(), can_merge_remove_vma(next) is only checked when
both can_merge_left and can_merge_right are true, and even then it only
reduces a 3-way merge to a 2-way merge into prev:

mm/vma.c:vma_merge_new_range() {
	...
	if (can_merge_right) {
		vmg->end = next->vm_end;
		vmg->target = next;
	}

	if (can_merge_left) {
		vmg->start = prev->vm_start;
		vmg->target = prev;
		...
		if (can_merge_right && !can_merge_remove_vma(next))
			vmg->end = end;
	...
}

Because vma_merge_new_range() is called by __mmap_region() and copy_vma() in
addition to vma_merge_extend(), allowing VMA_MIXEDMAP_BIT here affects two
other paths:

1. In __mmap_region(), when a driver's f_op->mmap_prepare callback sets
VMA_MIXEDMAP_BIT (for example via mmap_action_map_kernel_pages() ->
call_mmap_prepare() -> mmap_action_prepare() -> map_kernel_pages_prepare())
adjacent to a compatible existing VMA, vma_merge_new_range() will now expand
prev or next instead of returning NULL:

mm/vma.c:__mmap_region() {
	...
	if (map.prev || map.next) {
		VMG_MMAP_STATE(vmg, &map, /* vma = */ NULL);

		vma = vma_merge_new_range(&vmg);
	}

	if (!vma) {
		error = __mmap_new_vma(&map, &vma, &desc.action);
		if (error)
			goto unacct_error;
		allocated_new = true;
	}

	if (have_mmap_prepare && !map_is_anon(&map) && allocated_new)
		set_vma_user_defined_fields(vma, &map);

	__mmap_complete(&map, vma);

	if (have_mmap_prepare && allocated_new) {
		error = mmap_action_complete(vma, &desc.action,
					     /*is_compat=*/false);
	...
}

When vma_merge_new_range() returns a merged VMA, allocated_new remains
false, skipping both set_vma_user_defined_fields() and
mmap_action_complete() (which skips map_kernel_pages_complete() and
call_vma_mapped()).

2. When moving a VM_MIXEDMAP VMA via mremap() with MREMAP_MAYMOVE or
MREMAP_FIXED adjacent to another compatible VM_MIXEDMAP VMA, move_vma() ->
copy_vma_and_data() -> copy_vma() calls vma_merge_copied_range() ->
vma_merge_new_range(). The moved range is merged into the adjacent VMA
without checking can_merge_remove_vma(vmg->copied_from) and without calling
vm_ops->open() in copy_vma(), after which move_vma() calls
unmap_source_vma() -> do_vmi_munmap() and invokes vm_ops->close() on the
source VMA.

Also, since vma_merge_existing_range() still rejects all VMA_SPECIAL_FLAGS
upfront, is the VMA_MIXEDMAP_BIT check in can_merge_remove_vma() ever
reachable from vma_merge_existing_range()?

> +
>  /*
>   * vma_merge_new_range - Attempt to merge a new VMA into address space
>   *
> @@ -1152,9 +1172,7 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg)
>  
>  	vmg->state = VMA_MERGE_NOMERGE;
>  
> -	/* Special VMAs are unmergeable, also if no prev/next. */
> -	if (vma_flags_test_any_mask(&vmg->vma_flags, VMA_SPECIAL_FLAGS) ||
> -	    (!prev && !next))
> +	if (!can_merge_vma(vmg))
>  		return NULL;

[Severity: Medium]
Does this change cause test_vma_merge_special_flags() in
tools/testing/vma/tests/merge.c to fail on the VMA_MIXEDMAP_BIT iteration?

In test_vma_merge_special_flags(), the loop tests each flag in
special_flags[] (including VMA_MIXEDMAP_BIT) against merge_new() ->
vma_merge_new_range() when adjacent to vma_left:

tools/testing/vma/tests/merge.c:test_vma_merge_special_flags() {
	...
	vmg_set_range(&vmg, 0x3000, 0x4000, 3, vma_flags);
	for (i = 0; i < ARRAY_SIZE(special_flags); i++) {
		vma_flag_t special_flag = special_flags[i];
		vma_flags_t flags = vma_flags;

		vma_flags_set(&flags, special_flag);
		vma_left->flags = flags;
		vmg.vma_flags = flags;
		vma = merge_new(&vmg);
		ASSERT_EQ(vma, NULL);
		ASSERT_EQ(vmg.state, VMA_MERGE_NOMERGE);
	}
	...
}

Because can_merge_vma() allows VMA_MIXEDMAP_BIT and vmg.prev is vma_left
with matching flags and pgoff, can_vma_merge_left() and vma_expand()
succeed, returning vma_left with VMA_MERGE_SUCCESS and tripping the
assertions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023023106.515365-1-luizcap@redhat.com?part=1