Message-ID: <a8886e8d-b335-45fa-abaa-559c81247e89@lucifer.local>
Date: Thu, 10 Jul 2025 11:41:51 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Xu <peterx@...hat.com>, Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, Rik van Riel <riel@...riel.com>,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 09/10] mm/mremap: permit mremap() move of multiple VMAs

On Wed, Jul 09, 2025 at 02:13:41PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@...cle.com> [250707 01:28]:
> > Historically we've made it a uAPI requirement that mremap() may only
> > operate on a single VMA at a time.
> >
> > For instances where VMAs need to be resized, this makes sense, as it
> > becomes very difficult to determine what a user actually wants should they
> > indicate a desire to expand or shrink the size of multiple VMAs (truncate?
> > Adjust sizes individually? Some other strategy?).
> >
> > However, in instances where a user is moving VMAs, it is restrictive to
> > disallow this.
> >
> > This is especially the case when anonymous mapping remap may or may not be
> > mergeable depending on whether VMAs have or have not been faulted due to
> > anon_vma assignment and folio index alignment with vma->vm_pgoff.
> >
> > Often this can result in surprising impact where a moved region is faulted,
> > then moved back and a user fails to observe a merge from otherwise
> > compatible, adjacent VMAs.
> >
> > This change allows such cases to work without the user having to be
> > cognizant of whether a prior mremap() move or other VMA operations has
> > resulted in VMA fragmentation.
> >
> > Having refactored mremap code to aggregate per-VMA and parameter checks, we
> > are now in a position to permit this kind of move.
> >
> > We do so by detecting if this is a move-only operation up-front, and then
> > utilising a separate code path via remap_move() rather than the ordinary
> > single-VMA path.
> >
> > There are two tasks that occur outside of the mmap write lock - userfaultfd
> > notification and population of unmapped regions of expanded VMAs should the
> > VMA be mlock()'d.
> >
> > The latter doesn't apply, as this is logic for a move only and thus no
> > expansion can take place. In the former case, we explicitly disallow
> > multi-VMA operations on uffd-armed VMAs.
> >
> > The mmap lock is never dropped in the move-only case, this only occurs on a
> > VMA shrink.
> >
> > We take care to handle cases where a VMA merge has occurred, by resetting
> > the VMA iterator in such instances.
> >
> > We needn't worry about self-merges, as in those cases we would, by
> > definition, not be spanning multiple VMAs. The overlapping range test is
> > performed on the whole range so specifically disallows this.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> >  mm/mremap.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 99 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index 28e776cddc08..2e6005e1d22c 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -69,6 +69,8 @@ struct vma_remap_struct {
> >  	enum mremap_type remap_type;	/* expand, shrink, etc. */
> >  	bool mmap_locked;		/* Is mm currently write-locked? */
> >  	unsigned long charged;		/* If VM_ACCOUNT, # pages to account. */
> > +	bool multi_vma;			/* Is >1 VMA being moved? */
> > +	bool vma_reset;			/* Was the VMA merged/unmap occur? */
>
> The name doesn't read well in code.  vmi_reset or reset_iter might be
> better, but I don't really mind it like this.

Yeah it is a bit odd I agree.

>
> >  };
> >
> >  static pud_t *get_old_pud(struct mm_struct *mm, unsigned long addr)
> > @@ -1111,6 +1113,7 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
> >
> >  	err = do_vmi_munmap(&vmi, mm, addr, len, vrm->uf_unmap, /* unlock= */false);
> >  	vrm->vma = NULL; /* Invalidated. */
> > +	vrm->vma_reset = true;
>
> I believe the munmap() operation leaves the vmi in the correct position
> to reuse, so this is cautious that costs an extra walk of the tree.  I
> don't think it's critical to performance, but if it is we can look here.
> It would have to be passed through which might be a pain.

Yeah I think this means we _always_ reset the VMI as you mention below, unless
MREMAP_DONT_UNMAP | MREMAP_FIXED is used.

It's right to invalidate the vrm->vma here, as this is the source VMA so is now
a dangling pointer.

I think the problem I was worried about here was a partial unmap causing a
split, and keep in mind we might be moving things backwards also.

But I don't think the _iterator_ should be invalidated by this actually right?
We'd still be in the correct position.

So yeah, I'll drop this.

>
> >  	if (err) {
> >  		/* OOM: unable to split vma, just get accounts right */
> >  		vm_acct_memory(len >> PAGE_SHIFT);
> > @@ -1181,6 +1184,7 @@ static int copy_vma_and_data(struct vma_remap_struct *vrm,
> >
> >  	new_vma = copy_vma(&vma, vrm->new_addr, vrm->new_len, new_pgoff,
> >  			   &pmc.need_rmap_locks);
> > +	vrm->vma_reset = vma != vrm->vma;
> >  	if (!new_vma) {
> >  		vrm_uncharge(vrm);
> >  		*new_vma_ptr = NULL;
> > @@ -1325,6 +1329,7 @@ static unsigned long shrink_vma(struct vma_remap_struct *vrm,
> >  	res = do_vmi_munmap(&vmi, mm, unmap_start, unmap_bytes,
> >  			    vrm->uf_unmap, drop_lock);
> >  	vrm->vma = NULL; /* Invalidated. */
> > +	vrm->vma_reset = true;
>
> Ditto here, lock depending..

We won't ever drop the lock in a move path to be clear. Only on shrink, which is
disallowed for multi VMA move (as is expand).

So probably this is overcautious and I'll drop it.

>
> >  	if (res)
> >  		return res;
> >
> > @@ -1362,6 +1367,7 @@ static unsigned long mremap_to(struct vma_remap_struct *vrm)
> >  		err = do_munmap(mm, vrm->new_addr, vrm->new_len,
> >  				vrm->uf_unmap_early);
> >  		vrm->vma = NULL; /* Invalidated. */
> > +		vrm->vma_reset = true;
>
> Pretty sure this one is needed, regardless of passing through (and
> updating this call).

Yes this one for sure.

>
> >  		if (err)
> >  			return err;
> >
> > @@ -1581,6 +1587,18 @@ static bool vrm_will_map_new(struct vma_remap_struct *vrm)
> >  	return false;
> >  }
> >
> > +/* Does this remap ONLY move mappings? */
> > +static bool vrm_move_only(struct vma_remap_struct *vrm)
> > +{
> > +	if (!vrm_implies_new_addr(vrm))
> > +		return false;
> > +
> > +	if (vrm->old_len != vrm->new_len)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static void notify_uffd(struct vma_remap_struct *vrm, bool failed)
> >  {
> >  	struct mm_struct *mm = current->mm;
> > @@ -1644,10 +1662,29 @@ static int check_prep_vma(struct vma_remap_struct *vrm)
> >  			(vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP)))
> >  		return -EINVAL;
> >
> > -	/* We can't remap across vm area boundaries */
> > +	/*
> > +	 * We can't remap across the end of VMAs, as another VMA may be
> > +	 * adjacent:
> > +	 *
> > +	 *       addr   vma->vm_end
> > +	 *  |-----.----------|
> > +	 *  |     .          |
> > +	 *  |-----.----------|
> > +	 *        .<--------->xxx>
> > +	 *            old_len
> > +	 *
> > +	 * We also require that vma->vm_start <= addr < vma->vm_end.
> > +	 */
> >  	if (old_len > vma->vm_end - addr)
> >  		return -EFAULT;
> >
> > +	/*
> > +	 * We can't support moving multiple uffd VMAs as notify requires mmap
> > +	 * lock to be dropped.
> > +	 */
> > +	if (vrm->multi_vma && userfaultfd_armed(vma))
> > +		return -EINVAL;
> > +
> >  	if (new_len <= old_len)
> >  		return 0;
> >
> > @@ -1744,6 +1781,57 @@ static unsigned long check_mremap_params(struct vma_remap_struct *vrm)
> >  	return 0;
> >  }
> >
> > +static unsigned long remap_move(struct vma_remap_struct *vrm)
> > +{
> > +	struct vm_area_struct *vma;
> > +	unsigned long start = vrm->addr;
> > +	unsigned long end = vrm->addr + vrm->old_len;
> > +	unsigned long new_addr = vrm->new_addr;
> > +	unsigned long prev_addr = start;
> > +	VMA_ITERATOR(vmi, current->mm, start);
> > +
> > +	/*
> > +	 * When moving VMAs we allow for batched moves across multiple VMAs,
> > +	 * with all VMAs in the input range [addr, addr + old_len) being moved
> > +	 * (and split as necessary).
> > +	 */
> > +	for_each_vma_range(vmi, vma, end) {
> > +		unsigned long addr = max(vma->vm_start, start);
> > +		unsigned long len = min(end, vma->vm_end) - addr;
> > +		unsigned long offset = addr - start;
> > +		unsigned long res;
> > +
> > +		/* Merged with self, move on. */
> > +		if (vrm->multi_vma && prev_addr == addr)
> > +			continue;
> > +
> > +		vrm->vma = vma;
> > +		vrm->addr = addr;
> > +		vrm->new_addr = new_addr + offset;
> > +		vrm->old_len = vrm->new_len = len;
> > +
> > +		res = check_prep_vma(vrm);
> > +		if (!res)
> > +			res = mremap_to(vrm);
> > +		if (IS_ERR_VALUE(res))
> > +			return res;
> > +
> > +		/* mmap lock is only dropped on shrink. */
> > +		VM_WARN_ON_ONCE(!vrm->mmap_locked);
> > +		/* This is a move, no expand should occur. */
> > +		VM_WARN_ON_ONCE(vrm->populate_expand);
> > +
> > +		if (vrm->vma_reset) {
> > +			vma_iter_reset(&vmi);
> > +			vrm->vma_reset = false;
> > +		}
>
> What code path results in vma_reset == false here?

Yeah that's a good point, only MREMAP_DONT_UNMAP | MREMAP_FIXED will fail to hit
it, so let's drop for unmaps.

I will test this is all good too.

>
> > +		vrm->multi_vma = true;
> > +		prev_addr = addr;
> > +	}
> > +
> > +	return new_addr;
> > +}
>
> The iterator use looks good.

Thanks!

>
> > +
> >  static unsigned long do_mremap(struct vma_remap_struct *vrm)
> >  {
> >  	struct mm_struct *mm = current->mm;
> > @@ -1761,13 +1849,17 @@ static unsigned long do_mremap(struct vma_remap_struct *vrm)
> >  		return -EINTR;
> >  	vrm->mmap_locked = true;
> >
> > -	vrm->vma = vma_lookup(current->mm, vrm->addr);
> > -	res = check_prep_vma(vrm);
> > -	if (res)
> > -		goto out;
> > +	if (vrm_move_only(vrm)) {
> > +		res = remap_move(vrm);
> > +	} else {
> > +		vrm->vma = vma_lookup(current->mm, vrm->addr);
> > +		res = check_prep_vma(vrm);
> > +		if (res)
> > +			goto out;
> >
> > -	/* Actually execute mremap. */
> > -	res = vrm_implies_new_addr(vrm) ? mremap_to(vrm) : mremap_at(vrm);
> > +		/* Actually execute mremap. */
> > +		res = vrm_implies_new_addr(vrm) ? mremap_to(vrm) : mremap_at(vrm);
> > +	}
> >
> >  out:
> >  	failed = IS_ERR_VALUE(res);
> > --
> > 2.50.0
> >

Powered by blists - more mailing lists