Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 00:07:11 +0100
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Rapoport <rppt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        "Liam R . Howlett" <Liam.Howlett@oracle.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v2] mm: userfaultfd: avoid passing an invalid range to
 vma_merge()
Message-ID: <20abea10-5307-498b-b9df-8f0b2fed1701@lucifer.local>
References: <20230515193232.67552-1-lstoakes@gmail.com>
 <ZGKjvceDfpKMxtfb@x1n>
 <bcfb0376-ccf4-4467-9da5-8ae6f1c86876@lucifer.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bcfb0376-ccf4-4467-9da5-8ae6f1c86876@lucifer.local>
Precedence: bulk

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]