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

[Peter Xu]

On Tue, May 16, 2023 at 11:15:54PM +0100, Lorenzo Stoakes wrote:
> I'll try to address this in a later series, I don't think there's much use
> in going round in circles on this. If you dislike that series, you're
> welcome to provide negative feedback there, I don't think there's much use
> in discussing further here.

I'm happy to read it, sorry if any of my wording was intruding, I didn't
mean so.

I think there's chance at least on generalizing vma flag change cases, even
though I'm not sure whether vma_merge() needs change.  Maybe it can be
another layer on top of it while keeping vma_merge() as is, but I can't tell.

> We've seen a regression on invalid input to vma_merge() (explicitly I mean
> triggering a VM_WARN_ON()) and VMA fragmentation you were not aware of
> here, that does not strike me as a great + clear interface.

Yes, the code needs time to read through, even the interface.  I don't
think I fully digested that myself.

[...]

> Ah the thanks you get for contributing a regression fix _and_ a repro - a
> nack :) will you at least give me some kind of a tag... or buy me a beer?
> ;)

I can. :)

We actually met on the conference, if I'll be able to meet you somewhere
that's what I can do.

I was probably hashing in the words, sorry about that if so, and thanks for
looking at this issue!  I appreciate both your assertion patch and the png
documentation file.

It's just that I feel irresponsible when we were talking about having vma
not merged correctly but then the discussion tried to end at there saying
it kept so so it's fine.  IMHO we should look into that problem or
something could be missing here.  Then when I was looking into that
not-merged issue I found that it's not uffd that's special.

> > Before that I'd like to know whether you agree that the new patch 1 (I'll
> > fixup the vma_prev() side effect) could be a better solution than the
> > current one, no matter whether we need a full revert or not.
> 
> In principle it looks fine actually (pending Liam's assessment), case 4/5
> should handle it, but I feel like we need a comment (perhaps only in commit
> msg) to make clear that we are ensuring that the inputs to vma_merge() are
> either clamped to VMAs or case 4/5.
> 
> Let's see what Liam thinks, then let me check it locally to give a final
> OK, if I may.

Sounds perfect here.  Thanks a lot.

-- 
Peter Xu