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

[Lorenzo Stoakes]

On Fri, Aug 08, 2025 at 03:34:13PM +0100, Lorenzo Stoakes wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >  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...

I have asked Andrew to update with a clear explanation of this (see [0]), and
made clear why I feel it's consistent for us to disallow this behaviour for
non-eligible VMAs while permitting it for eligible ones.

It means we can simply say 'for eligible pure moves, you may specify gaps
between or after VMAs spanning 1 or more VMAs'.

Cheers, Lorenzo

[0]: https://lore.kernel.org/linux-mm/df80b788-0546-4b78-a2fa-64d26e5a35b8@lucifer.local/