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, 14 Jun 2023 11:31:03 -0400
From:   Peter Xu <peterx@redhat.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Matthew Wilcox <willy@infradead.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Mike Rapoport <rppt@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        James Houghton <jthoughton@google.com>,
        Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH 4/7] mm/hugetlb: Prepare hugetlb_follow_page_mask() for
 FOLL_PIN
Message-ID: <ZIndN9isc4pTp2zK@x1n>
References: <20230613215346.1022773-1-peterx@redhat.com>
 <20230613215346.1022773-5-peterx@redhat.com>
 <533c32cf-9a18-1590-4d29-f076d6bd58c1@redhat.com>
 <ZInYh3cVUil9R/cf@x1n>
 <ef0f8e0e-cbce-7c7b-1b0e-c9d52ede7e0e@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ef0f8e0e-cbce-7c7b-1b0e-c9d52ede7e0e@redhat.com>
Precedence: bulk

On Wed, Jun 14, 2023 at 05:17:13PM +0200, David Hildenbrand wrote:
> On 14.06.23 17:11, Peter Xu wrote:
> > On Wed, Jun 14, 2023 at 04:57:37PM +0200, David Hildenbrand wrote:
> > > On 13.06.23 23:53, Peter Xu wrote:
> > > > It's coming, not yet, but soon.  Loose the restriction.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >    mm/hugetlb.c | 7 -------
> > > >    1 file changed, 7 deletions(-)
> > > > 
> > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > > > index f037eaf9d819..31d8f18bc2e4 100644
> > > > --- a/mm/hugetlb.c
> > > > +++ b/mm/hugetlb.c
> > > > @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
> > > >    	spinlock_t *ptl;
> > > >    	pte_t *pte, entry;
> > > > -	/*
> > > > -	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
> > > > -	 * follow_hugetlb_page().
> > > > -	 */
> > > > -	if (WARN_ON_ONCE(flags & FOLL_PIN))
> > > > -		return NULL;
> > > > -
> > > >    	hugetlb_vma_lock_read(vma);
> > > >    	pte = hugetlb_walk(vma, haddr, huge_page_size(h));
> > > >    	if (!pte)
> > > Did you fix why the warning was placed there in the first place? (IIRC, at
> > > least unsharing support needs to be added, maybe more)
> > 
> > Feel free to have a look at patch 2 - it should be done there, hopefully in
> > the right way.  And IIUC it could be a bug to not do that before (besides
> > CoR there was also the pgtable permission checks that was missing).  More
> > details in patch 2's commit message.  Thanks,
> 
> Oh, that slipped my eyes (unsharing is not really a permission check) -- and

I think it is still a "permission check"?  It means, we forbid anyone R/O
taking the page if it's not exclusively owned, just like we forbid anyone
RW taking the page if it's not writable?

It's just that the permission check only applies to PIN which follow_page()
doesn't yet care, so it won't ever trigger.

> the patch description could have been more explicit about why we can now
> lift the restrictions.
> 
> For the records: we don't use CoR terminology upstream. As suggested by
> John, we use "GUP-triggered unsharing".

Sure.

> 
> As unsharing only applies to FOLL_PIN, it doesn't quite fit into patch #2.
> Either move that to this patch or squash both.

Sure, no strong opinions here.

The plan is _if_ someone wants to backport patch 2, this patch should not
be part of it.  But then maybe it makes more sense to move the CoR change
there into this one, not because "it's not permission check", but because
CoR is not relevant in follow_page(), so not relevant to a backport.

-- 
Peter Xu