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

[Lorenzo Stoakes]

On Tue, May 16, 2023 at 06:32:38PM -0400, Peter Xu wrote:
> 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.

No it's fine, I've perhaps not been clear, but this has all inspired an
upcoming change so it's all good :)

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

Indeed, will dig in!

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

Yeah was nice to meet! And only being tongue in cheek, I actually at this
point do think your suggestion should replace my patch.

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

Sure, we should try to keep all invocations as close as possible if we
can. It does seem as you say that uffd got left behind on this.

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

Great!

>
> --
> Peter Xu
>