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, 13 Jul 2022 12:10:04 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Mike Kravetz <mike.kravetz@oracle.com>,
        Axel Rasmussen <axelrasmussen@google.com>
Cc:     Miaohe Lin <linmiaohe@huawei.com>, akpm@linux-foundation.org,
        songmuchun@bytedance.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/hugetlb: avoid corrupting page->mapping in
 hugetlb_mcopy_atomic_pte
Message-ID: <Ys7uXHdzzFezUub5@xz-m1.local>
References: <20220712130542.18836-1-linmiaohe@huawei.com>
 <Ys2xyCUnqpJt0eIo@monkey>
 <Ys7ViSsOxLaQJIfy@xz-m1.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Ys7ViSsOxLaQJIfy@xz-m1.local>
Precedence: bulk

On Wed, Jul 13, 2022 at 10:24:09AM -0400, Peter Xu wrote:
> On Tue, Jul 12, 2022 at 10:39:20AM -0700, Mike Kravetz wrote:
> > On 07/12/22 21:05, Miaohe Lin wrote:
> > > In MCOPY_ATOMIC_CONTINUE case with a non-shared VMA, pages in the page
> > > cache are installed in the ptes. But hugepage_add_new_anon_rmap is called
> > > for them mistakenly because they're not vm_shared. This will corrupt the
> > > page->mapping used by page cache code.
> > > 
> > > Fixes: f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE ioctl")
> > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > > ---
> > >  mm/hugetlb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > This looks correct to me.
> > 
> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> > 
> > However, I am having a hard time wrapping my head around how UFFDIO_CONTINUE
> > should work on non-anon private mappings.  For example, a private mapping of
> > a hugetlbfs file.  I think we just map the page in the file/cache and do not
> > set the write bit in the pte.  So, yes we would want page_dup_file_rmap()
> > in this case as shown below.
> > 
> > Adding Axel and Peter on Cc: as they were more involved in adding that code
> > and the design of UFFDIO_CONTINUE.
> 
> Yes the change makes sense to me too.  There's just one thing to check on
> whether minor mode should support private mappings at all as it's probably
> not in the major goal of when it's proposed.
> 
> I don't see why it can't logically, but I think we should have failed the
> uffdio-register already somewhere before when the vma was private and
> registered with minor mode.  It's just that I cannot quickly find it in the
> code anywhere..  ideally it should be checked in vma_can_userfault() but it
> seems not.
> 
> Axel?
> 
> PS: the minor mode man page update seems to be still missing.

Oh I should have done a pull first on the man-page repo..

From the man page indeed I didn't see anything mentioned on not allowing
private mappings.  There's the example given on using two mappings for
modifying pages but logically that applies to private mappings too - we
could have mapped the uffd region with private mappings but the other one
shared, then we can modify page caches but later after pte installed it'll
trigger cow for writes.

So I think we need to confirm whether private mappings are supported. If
no, we should be crystal clear in both the code and man page (we probably
want a follow up patch to man-page to mention that too?).  If yes, we'll
need Miaohe's patch and also make sure they're enabled in the current code
path.  We'll also want to set test_uffdio_minor=1 for "hugetlb" test case
in the userfaultfd kselftest (currently it's not there).

-- 
Peter Xu