Re: [PATCH 6.17 2/3] mm/mremap: catch invalid multi VMA moves earlier

[Vlastimil Babka]

On 8/3/25 13:11, Lorenzo Stoakes wrote:
> In remap_move() we must account for both a single VMA case, where we are
> permitted to move a single VMA regardless of multi-VMA move eligiblity, and
> multiple VMAs which, of course, must be eligible for such an operation.
> 
> We determine this via vma_multi_allowed().
> 
> Currently, if the first VMA is not eligible, but others are, we will move
> the first then return an error. This is not ideal, as we are performing an
> operation which we don't need to do which has an impact on the memory
> mapping.
> 
> We can very easily determine if this is a multi VMA move prior to the move
> of the first VMA, by checking vma->vm_end vs. the specified end address.
> 
> Therefore this patch does so, and as a result eliminates unnecessary logic
> around tracking whether the first VMA was permitted or not.
> 
> This is most useful for cases where a user attempts to erroneously move
> mutliple VMAs which are not eligible for non-transient reasons - for
> instance, UFFD-armed VMAs, or file-backed VMAs backed by a file system or
> driver which specifies a custom f_op->get_unmapped_area.
> 
> In the less likely instance of a failure due to transient issues such as
> out of memory or mapping limits being hit, the issue is already likely
> fatal and so the fact the operation may be partially complete is
> acceptable.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
>  mm/mremap.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 46f9f3160dff..f61a9ea0b244 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1816,10 +1816,11 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  	unsigned long start = vrm->addr;
>  	unsigned long end = vrm->addr + vrm->old_len;
>  	unsigned long new_addr = vrm->new_addr;
> -	bool allowed = true, seen_vma = false;
>  	unsigned long target_addr = new_addr;
>  	unsigned long res = -EFAULT;
>  	unsigned long last_end;
> +	bool seen_vma = false;
> +
>  	VMA_ITERATOR(vmi, current->mm, start);
>  
>  	/*
> @@ -1833,9 +1834,6 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  		unsigned long len = min(end, vma->vm_end) - addr;
>  		unsigned long offset, res_vma;
>  
> -		if (!allowed)
> -			return -EFAULT;
> -
>  		/* No gap permitted at the start of the range. */
>  		if (!seen_vma && start < vma->vm_start)
>  			return -EFAULT;
> @@ -1863,9 +1861,14 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  		vrm->new_addr = target_addr + offset;
>  		vrm->old_len = vrm->new_len = len;
>  
> -		allowed = vma_multi_allowed(vma);
> -		if (seen_vma && !allowed)
> -			return -EFAULT;
> +		if (!vma_multi_allowed(vma)) {
> +			/* This is not the first VMA, abort immediately. */
> +			if (seen_vma)
> +				return -EFAULT;
> +			/* This is the first, but there are more, abort. */
> +			if (vma->vm_end < end)
> +				return -EFAULT;

Hm there can just also be a gap, and we permit gaps at the end (unlike at
the start), right?

So we might be denying a multi vma mremap for !vma_multi_allowed() reasons
even if it's a single vma and a gap.

AFAICS this is not regressing the behavior prior to d23cb648e365
("mm/mremap: permit mremap() move of multiple VMAs") as such mremap() would
be denied anyway by the "/* We can't remap across vm area boundaries */"
check in check_prep_vma().

So the question is just if we want this odd corner case to behave like this,
and if yes then be more explicit about it perhaps.

> +		}
>  
>  		res_vma = check_prep_vma(vrm);
>  		if (!res_vma)
> @@ -1874,7 +1877,8 @@ static unsigned long remap_move(struct vma_remap_struct *vrm)
>  			return res_vma;
>  
>  		if (!seen_vma) {
> -			VM_WARN_ON_ONCE(allowed && res_vma != new_addr);
> +			VM_WARN_ON_ONCE(vma_multi_allowed(vma) &&
> +					res_vma != new_addr);
>  			res = res_vma;
>  		}
>