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:   Wed, 19 Apr 2023 19:45:06 +0100
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Jason Gunthorpe <jgg@nvidia.com>, Jens Axboe <axboe@kernel.dk>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Pavel Begunkov <asml.silence@gmail.com>,
        io-uring@vger.kernel.org
Subject: Re: [PATCH v4 4/6] io_uring: rsrc: avoid use of vmas parameter in
 pin_user_pages()
Message-ID: <8bf0df41-27ef-4305-b424-e43045a6d68d@lucifer.local>
References: <936e8f52-00be-6721-cb3e-42338f2ecc2f@kernel.dk>
 <c2e22383-43ee-5cf0-9dc7-7cd05d01ecfb@kernel.dk>
 <f82b9025-a586-44c7-9941-8140c04a4ccc@lucifer.local>
 <69f48cc6-8fc6-0c49-5a79-6c7d248e4ad5@kernel.dk>
 <bec03e0f-a0f9-43c3-870b-be406ca848b9@lucifer.local>
 <8af483d2-0d3d-5ece-fb1d-a3654411752b@kernel.dk>
 <d601ca0c-d9b8-4e5d-a047-98f2d1c65eb9@lucifer.local>
 <ZEAxhHx/4Ql6AMt2@casper.infradead.org>
 <ZEAx90C2lDMJIux1@nvidia.com>
 <ZEA0dbV+qIBSD0mG@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZEA0dbV+qIBSD0mG@casper.infradead.org>
Precedence: bulk

On Wed, Apr 19, 2023 at 07:35:33PM +0100, Matthew Wilcox wrote:
> On Wed, Apr 19, 2023 at 03:24:55PM -0300, Jason Gunthorpe wrote:
> > On Wed, Apr 19, 2023 at 07:23:00PM +0100, Matthew Wilcox wrote:
> > > On Wed, Apr 19, 2023 at 07:18:26PM +0100, Lorenzo Stoakes wrote:
> > > > So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I
> > > > would still need to come along and delete a bunch of your code
> > > > afterwards. And unfortunately Pavel's recent change which insists on not
> > > > having different vm_file's across VMAs for the buffer would have to be
> > > > reverted so I expect it might not be entirely without discussion.
> > >
> > > I don't even understand why Pavel wanted to make this change.  The
> > > commit log really doesn't say.
> > >
> > > commit edd478269640
> > > Author: Pavel Begunkov <asml.silence@gmail.com>
> > > Date:   Wed Feb 22 14:36:48 2023 +0000
> > >
> > >     io_uring/rsrc: disallow multi-source reg buffers
> > >
> > >     If two or more mappings go back to back to each other they can be passed
> > >     into io_uring to be registered as a single registered buffer. That would
> > >     even work if mappings came from different sources, e.g. it's possible to
> > >     mix in this way anon pages and pages from shmem or hugetlb. That is not
> > >     a problem but it'd rather be less prone if we forbid such mixing.
> > >
> > >     Cc: <stable@vger.kernel.org>
> > >     Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > >     Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > >
> > > It even says "That is not a problem"!  So why was this patch merged
> > > if it's not fixing a problem?
> > >
> > > It's now standing in the way of an actual cleanup.  So why don't we
> > > revert it?  There must be more to it than this ...
> >
> > https://lore.kernel.org/all/61ded378-51a8-1dcb-b631-fda1903248a9@gmail.com/
>
> So um, it's disallowed because Pavel couldn't understand why it
> should be allowed?  This gets less and less convincing.
>
> FWIW, what I was suggesting was that we should have a FOLL_SINGLE_VMA
> flag, which would use our shiny new VMA lock infrastructure to look
> up and lock _one_ VMA instead of having the caller take the mmap_lock.
> Passing that flag would be a tighter restriction that Pavel implemented,
> but would certainly relieve some of his mental load.
>
> By the way, even if all pages are from the same VMA, they may still be a
> mixture of anon and file pages; think a MAP_PRIVATE of a file when
> only some pages have been written to.  Or an anon MAP_SHARED which is
> accessible by a child process.

Indeed, my motive for the series came out of a conversation with you about
vmas being odd (thanks! :), however I did end up feeling FOLL_SINGLE_VMA
would be too restricted and would break the uAPI.

For example, imagine if a user (yes it'd be weird) mlock'd some pages in a
buffer and not others, then we'd break their use case. Also (perhaps?) more
feasibly, a user might mix hugetlb and anon pages. So I think that'd be too
restrictive here.

However the idea of just essentially taking what Jens has had to do
open-coded and putting it into GUP as a whole really feels like the right
thing to do.

I do like the idea of a FOLL_SINGLE_VMA for other use cases though, the
majority of which want one and one page only. Perhaps worth taking the
helper added in this series (get_user_page_vma_remote() from [1]) and
replacing it with an a full GUP function which has an interface explicitly
for this common single page/vma case.

[1]:https://lore.kernel.org/linux-mm/7c6f1ae88320bf11d2f583178a3d9e653e06ac63.1681831798.git.lstoakes@gmail.com/