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

[Liam R. Howlett]

* Peter Xu <peterx@xxxxxxxxxx> [230517 09:50]:

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

I don't get your use of split here. __vma_adjust() used to be used by
split, but it never split a VMA.  vma_merge() is not used by split at
all.

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