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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Aug 08, 2025 at 04:19:09PM +0200, Vlastimil Babka wrote:
> On 8/3/25 13:11, Lorenzo Stoakes wrote:
[snip]
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  mm/mremap.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
[snip]
> > @@ -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?

I don't think we should allow a single VMA with gap, it's actually more
correct to maintain existing behavour in this case.

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

This is therfore a useful exercise in preventing us from permitting this
case I think.

>
> 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().

Yup.

And this code is _only_ called for MREMAP_FIXED. So nothing else is impacted.

>
> 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.

We definitely do IMO. There's no reason to change this behaviour.

The end gap thing in multi was more a product of 'why not permit it' but
now is more a case of 'it means we don't have to go check or fail
partially'.

So I think this is fine.

>
> > +		}
> >
> >  		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;
> >  		}
> >
>

I can update the commit msg accordingly...