Re: [PATCH v2] mm: userfaultfd: avoid passing an invalid range to vma_merge()

[Peter Xu <peterx@redhat.com>]

On Tue, May 16, 2023 at 06:52:51PM -0400, Liam R. Howlett wrote:
> * Peter Xu <peterx@redhat.com> [230516 16:12]:
> ...
> 
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  fs/userfaultfd.c | 27 +++++++++++++++++++++------
> > > >  1 file changed, 21 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > > index 0fd96d6e39ce..7eb88bc74d00 100644
> > > > --- a/fs/userfaultfd.c
> > > > +++ b/fs/userfaultfd.c
> > > > @@ -1458,10 +1458,17 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > >  	BUG_ON(!found);
> > > >  
> > > >  	vma_iter_set(&vmi, start);
> > > > -	prev = vma_prev(&vmi);
> > > > +	vma = vma_find(&vmi, end);
> > > > +	if (!vma)
> > > > +		goto out_unlock;
> > > > +
> > > > +	if (vma->vm_start < start)
> > > > +		prev = vma;
> > > > +	else
> > > > +		prev = vma_prev(&vmi);
> > > >  
> > > >  	ret = 0;
> > > > -	for_each_vma_range(vmi, vma, end) {
> > > > +	do {
> > > 
> > > The iterator may be off by one, depending on if vma_prev() is called or
> > > not.
> > > 
> > > Perhaps:
> > > 	prev = vma_prev(&vmi); /* could be wrong, or null */
> > > 	vma = vma_find(&vmi, end);
> > > 	if (!vma)
> > > 		goto out_unlock;
> > > 
> > > 	if (vma->vm_start < start)
> > > 		prev = vma;
> > > 
> > > now we know we are at vma with the iterator..
> > > 	ret = 0
> > > 	do{
> > > 	...
> > 
> > Will do, thanks.
> > 
> > I think I got trapped similarly when I was looking at xarray months ago
> > where xarray also had similar side effects to have off-by-one the iterator
> > behavior.
> > 
> > Do you think it'll make sense to have something describing such side
> > effects for maple tree (or the current vma api), or.. maybe there's already
> > some but I just didn't really know?
> 
> Well, it's an iterator so I though a position was implied.  But I think
> the documentation is lacking on the vma_iterator interface and I should
> fix that.

Thanks.

> 
> ...
> 
> > > > From: Peter Xu <peterx@redhat.com>
> > > > Date: Tue, 16 May 2023 09:39:38 -0400
> > > > Subject: [PATCH 2/2] mm/uffd: Allow vma to merge as much as possible
> > > > 
> > > > We used to not pass in the pgoff correctly when register/unregister uffd
> > > > regions, it caused incorrect behavior on vma merging.
> > > > 
> > > > For example, when we have:
> > > > 
> > > >   vma1(range 0-9, with uffd), vma2(range 10-19, no uffd)
> > > > 
> > > > Then someone unregisters uffd on range (5-9), it should become:
> > > > 
> > > >   vma1(range 0-4, with uffd), vma2(range 5-19, no uffd)
> > > > 
> > > > But with current code it's:
> > > > 
> > > >   vma1(range 0-4, with uffd), vma3(range 5-9, no uffd), vma2(range 10-19, no uffd)
> > > > 
> > > > This patch allows such merge to happen correctly.
> > > > 
> > > > This behavior seems to have existed since the 1st day of uffd, keep it just
> > > > as a performance optmization and not copy stable.
> > > > 
> > > > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > > > Cc: Mike Rapoport (IBM) <rppt@kernel.org>
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  fs/userfaultfd.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > > index 7eb88bc74d00..891048b4799f 100644
> > > > --- a/fs/userfaultfd.c
> > > > +++ b/fs/userfaultfd.c
> > > > @@ -1332,6 +1332,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > >  	bool basic_ioctls;
> > > >  	unsigned long start, end, vma_end;
> > > >  	struct vma_iterator vmi;
> > > > +	pgoff_t pgoff;
> > > >  
> > > >  	user_uffdio_register = (struct uffdio_register __user *) arg;
> > > >  
> > > > @@ -1489,8 +1490,9 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > >  		vma_end = min(end, vma->vm_end);
> > > >  
> > > >  		new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> > > > +		pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> > > 
> > > I don't think this is safe.  You are telling vma_merge() something that
> > > is not true and will result in can_vma_merge_before() passing.  I mean,
> > > sure it will become true after you split (unless you can't?), but I
> > > don't know if you can just merge a VMA that doesn't pass
> > > can_vma_merge_before(), even for a short period?
> > 
> > I must admit I'm not really that handy yet to vma codes, so I could miss
> > something obvious.
> > 
> > My reasoning comes from two parts that this pgoff looks all fine:
> > 
> > 1) It's documented in vma_merge() in that:
> > 
> >  * Given a mapping request (addr,end,vm_flags,file,pgoff,anon_name),
> >  * figure out ...
> > 
> >   So fundamentally this pgoff is part of the mapping request paired with
> >   all the rest of the information.  AFAICT it means it must match with what
> >   "addr" is describing in VA address space.  That's why I think offseting
> >   it makes sense here.
> > 
> >   It also matches my understanding in vma_merge() code on how the pgoff is
> >   used.
> > 
> > 2) Uffd is nothing special in this regard, namely:
> > 
> >    mbind_range():
> > 
> > 	pgoff = vma->vm_pgoff + ((vmstart - vma->vm_start) >> PAGE_SHIFT);
> > 	merged = vma_merge(vmi, vma->vm_mm, *prev, vmstart, vmend, vma->vm_flags,
> > 			 vma->anon_vma, vma->vm_file, pgoff, new_pol,
> > 			 vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > 
> >    mlock_fixup():
> >    
> > 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> > 	*prev = vma_merge(vmi, mm, *prev, start, end, newflags,
> > 			vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> > 			vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > 
> >    mprotect_fixup():
> > 
> > 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> > 	*pprev = vma_merge(vmi, mm, *pprev, start, end, newflags,
> > 			   vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> > 			   vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > 
> > I had a feeling that it's just something overlooked in the initial proposal
> > of uffd, but maybe I missed something important?
> 
> I think you are correct.  It's worth noting that all of these skip
> splitting if merging succeeds.

Yes, IIUC that's what we want because vma_merge() just handles everything
there (including split, or say, vma range adjustments) if any !NULL
returned.

> 
> We know it won't match case 1-4 (we have a current vma).  We then pass
> in vma_end = min(end, vma->vm_end);

Case 4 seems still possible and should be the case that mentioned in the
patch 2, iiuc.  But yes I think vma_end calculation is needed, afaik it is
to cover the last iteration, where that's the only place possible that we
may operate on "end" (where < vma->vm_end) rather than "vma->vm_end".  It
actually pairs with the initial "start" adjustment to me.

> 
> vma_lookup() will only be called if end == vma->vm_end, so next will not
> be set (and found) unless it is adjacent to the current vma and the vma
> in question does not need to be split anyways.
> 
> I also see that we use pgoff+pglen in the check, which avoids my concern
> above.

Right.

It seems so far all concerns are more or less ruled out.  I'll prepare a
formal patchset, we can continue the discussion there.

Thanks,

-- 
Peter Xu