Message-ID: <20230516225251.xwmz2oyebo7k56ys@revolver>
Date:   Tue, 16 May 2023 18:52:51 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Lorenzo Stoakes <lstoakes@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v2] mm: userfaultfd: avoid passing an invalid range to
 vma_merge()

* Peter Xu <peterx@...hat.com> [230516 16:12]:
...

> > > 
> > > Signed-off-by: Peter Xu <peterx@...hat.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.

...

> > > From: Peter Xu <peterx@...hat.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@...hat.com>
> > > Cc: Mike Rapoport (IBM) <rppt@...nel.org>
> > > Signed-off-by: Peter Xu <peterx@...hat.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.

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);

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.

> 
> > 
> > >  		prev = vma_merge(&vmi, mm, prev, start, vma_end, new_flags,
> > > -				 vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > > +				 vma->anon_vma, vma->vm_file, pgoff,
> > >  				 vma_policy(vma),
> > >  				 ((struct vm_userfaultfd_ctx){ ctx }),
> > >  				 anon_vma_name(vma));
> > > @@ -1570,6 +1572,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> > >  	unsigned long start, end, vma_end;
> > >  	const void __user *buf = (void __user *)arg;
> > >  	struct vma_iterator vmi;
> > > +	pgoff_t pgoff;
> > >  
> > >  	ret = -EFAULT;
> > >  	if (copy_from_user(&uffdio_unregister, buf, sizeof(uffdio_unregister)))
> > > @@ -1677,8 +1680,9 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> > >  			uffd_wp_range(vma, start, vma_end - start, false);
> > >  
> > >  		new_flags = vma->vm_flags & ~__VM_UFFD_FLAGS;
> > > +		pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
> > >  		prev = vma_merge(&vmi, mm, prev, start, vma_end, new_flags,
> > > -				 vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > > +				 vma->anon_vma, vma->vm_file, pgoff,
> > >  				 vma_policy(vma),
> > >  				 NULL_VM_UFFD_CTX, anon_vma_name(vma));
> > >  		if (prev) {
> > > -- 
> > > 2.39.1
> > > ---8<---
> > > 
> > > -- 
> > > Peter Xu
> > > 
> > 
> 
> -- 
> Peter Xu
> 

Powered by blists - more mailing lists