On Mon, May 15, 2023 at 11:04:27PM +0100, Lorenzo Stoakes wrote: [snip] > > 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 [0] 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 3 VMAs. 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!) [0]: https://gist.github.com/lorenzo-stoakes/a11a10f5f479e7a977fc456331266e0e [snip]
