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 10:24:09 -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,
        Axel Rasmussen <axelrasmussen@google.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: <Ys7ViSsOxLaQJIfy@xz-m1.local>
References: <20220712130542.18836-1-linmiaohe@huawei.com>
 <Ys2xyCUnqpJt0eIo@monkey>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Ys2xyCUnqpJt0eIo@monkey>
Precedence: bulk

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.

-- 
Peter Xu