Re: [PATCH] mm/mremap: enforce the driver's page size on resize and move

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/mremap: move_vma() checks alignment after mremap_to()
  unmaps and shrinks
- [Medium] mm/mremap: vrm_can_expand_in_place() causes -ENOMEM on
  unaligned expand
- [Low] mm/mremap: duplicated subject line at the start of commit
  message body
--

commit 0a9d27ac6fbd8948362259538484e3729c9ccd36
Author: Shao Mingyin <shao.mingyin@zte.com.cn>

mm/mremap: enforce the driver's page size on resize and move

This commit validates vma_kernel_pagesize() alignment of VMA boundaries
when moving or expanding a mapping in mremap().

> mm/mremap: enforce the driver's page size on resize and move
>
> mm/mremap: enforce the driver's page size on resize and move

[Severity: Low]
This isn't a bug, but the subject line is duplicated at the start of the
commit message body.

> diff --git a/mm/mremap.c b/mm/mremap.c
> index fff0465f5e061..8a0a406bd78d4 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1406,6 +1406,17 @@ static unsigned long move_vma(struct vma_remap_struct *vrm)
>  	unsigned long hiwater_vm;
>  	int err;
>  
> +	/*
> +	 * A driver can impose a page size larger than PAGE_SIZE through
> +	 * vm_ops->pagesize(); device-dax exposes dev_dax->align this way and
> +	 * rejects any VMA whose start or end is not a multiple of it.  The
> +	 * destination VMA spans [new_addr, new_addr + new_len), so do not
> +	 * create it with a boundary the driver would reject.
> +	 */
> +	if ((vrm->new_addr | vrm->new_len) &
> +	    (vma_kernel_pagesize(vrm->vma) - 1))
> +		return -EINVAL;
> +
>  	err = prep_move_vma(vrm);
>  	if (err)
>  		return err;

[Severity: Medium]
Does checking vrm->new_addr and vrm->new_len in move_vma() run too late
when called from mremap_to()?

Before calling move_vma(), mremap_to() unmaps the destination range when
MREMAP_FIXED is set and shrinks the source VMA when MREMAP_SHRINK is set:

mm/mremap.c:mremap_to() {
	if (vrm->flags & MREMAP_FIXED) {
		...
		err = do_munmap(mm, vrm->new_addr, vrm->new_len,
				vrm->uf_unmap_early);
		...
	}

	if (vrm->remap_type == MREMAP_SHRINK) {
		err = shrink_vma(vrm, /* drop_lock= */false);
		...
	}
	...
	return move_vma(vrm);
}

If a caller passes a PAGE_SIZE-aligned new_addr or new_len that is not
aligned to vma_kernel_pagesize(vrm->vma), mremap() destroys mappings in
[new_addr, new_addr + new_len) and/or shrinks the source VMA before
failing with -EINVAL.

> @@ -1577,6 +1588,16 @@ static bool vrm_can_expand_in_place(struct vma_remap_struct *vrm)
>  	if (!vma_expandable(vrm->vma, vrm->delta))
>  		return false;
>  
> +	/*
> +	 * Growing in place only moves the end of the VMA.  A driver page
> +	 * size larger than PAGE_SIZE requires the new end to stay aligned;
> +	 * otherwise let the caller try a move, where move_vma() validates
> +	 * the destination instead.
> +	 */
> +	if ((vrm->vma->vm_end + vrm->delta) &
> +	    (vma_kernel_pagesize(vrm->vma) - 1))
> +		return false;
> +
>  	return true;
>  }

[Severity: Medium]
Does returning false from vrm_can_expand_in_place() cause expand_vma() to
return -ENOMEM instead of -EINVAL on an unaligned expansion?

When vrm_can_expand_in_place() returns false and MREMAP_MAYMOVE is not
set, expand_vma() immediately returns -ENOMEM even though virtual address
space is available in place and the failure is an invalid size alignment.
Even when MREMAP_MAYMOVE is set, because suffix_bytes == vrm->old_len here
and vrm->vma->vm_start is already aligned to vma_kernel_pagesize(vrm->vma),
an unaligned vrm->vma->vm_end + vrm->delta (= vrm->addr + vrm->new_len)
means either vrm->new_len is unaligned or vrm->addr is an unaligned split
point, so the fallback move can never succeed and may also fail with
-ENOMEM in vrm_set_new_addr() or check_map_count_against_split().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115004700194_SpQtdnE3JL6bzx51l-a5@zte.com.cn?part=1