Re: [PATCH v2] mm: userfaultfd: avoid passing an invalid range to vma_merge()
From: Lorenzo Stoakes
Date: Mon May 15 2023 - 19:07:38 EST
On Mon, May 15, 2023 at 11:04:27PM +0100, Lorenzo Stoakes wrote:
> > Could you explain a bit why we don't need to merge in this case?
> > I'm considering, 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), iiuc it should become:
> > vma1(range 0-4, with uffd), vma2(range 5-19, no uffd)
> > But if no merge here it's:
> > vma1(range 0-4, with uffd), vma3(range 5-9, no uffd), vma2(range 10-19, no uffd)
> > Maybe I missed something?
> There's something really, really wrong with this. It simply isn't valid to
> invoke vma_merge() over an existing VMA that != prev where you're not
> specifying addr = vma->vm_start, end == vma->vm_end.
> This seems like you're relying on:-
> CCCCCNNNNN -> CCNNNNNNNN
> By specifying parameters that are compatible with N even though you're only
> partially spanning C?
> This is crazy, and isn't how this should be used. vma_merge() is not
> supposed to do partial merges. If it works (presumably it does) this is not
> by design unless I've lost my mind and I (and others) have somehow not
> noticed this??
> I think you're right that now we'll end up with more fragmentation, but
> what you're suggesting is not how vma_merge() is supposed to work.
> As I said above, giving vma_merge() invalid parameters is very dangerous as
> you could end up merging over empty ranges in theory (and could otherwise
> have corruption).
> I guess we should probably be passing 0 to the last parameter in
> split_vma() here then to ensure we do a merge pass too. Will experiment
> with this.
> I'm confused as to how the remove from case 8 is not proceeding. I'll look
> into this some more...
> Happy to be corrected if I'm misconstruing this!
OK, so I wrote a small program to do perform exactly this case  and it seems
that the outcome is the same before and after this patch - vma_merge() is
clearly rejecting the case 8 merge (phew!) and in both instances you end up with
So this patch doesn't change this behaviour and everything is as it was
before. Ideally we'd let it go for another pass, so maybe we should change the
split to add a new VMA _afterwards_. Will experiment with that, separately.
But looks like the patch is good as it is.
(if you notice something wrong with the repro, etc. do let me know!)