Message-ID: <CAJHvVcgqb6R6ePMbgmA8LpMpMgrGWie9ZCTuR4MA77GTvi4XAw@mail.gmail.com>
Date:   Wed, 13 Jul 2022 15:46:06 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Muchun Song <songmuchun@...edance.com>,
        Linux MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/hugetlb: avoid corrupting page->mapping in hugetlb_mcopy_atomic_pte

I think there is a small mistake in this patch.

Consider the non-minor-fault case. We have this block:

/* Add shared, newly allocated pages to the page cache. */
if (vm_shared && !is_continue) {
        /* ... */
}

In here, we've added the newly allocated page to the page cache, and
we've set this page_in_pagecache flag to true. But we *do not* setup
rmap for this page in this block. I think in this case, the patch will
cause us to do the wrong thing: we should hugepage_add_new_anon_rmap()
further down, but with this patch we dup instead.

On Wed, Jul 13, 2022 at 9:10 AM Peter Xu <peterx@...hat.com> wrote:
>
> 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@...wei.com>
> > > > ---
> > > >  mm/hugetlb.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > This looks correct to me.
> > >
> > > Reviewed-by: Mike Kravetz <mike.kravetz@...cle.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).

So, originally when I proposed minor fault handling, I was expecting
to only support VM_SHARED VMAs. Indeed, I too have a hard time
imagining how one might benefit from using it with a private mapping.
If my memory serves this restriction was relaxed due to feedback on
the original RFC proposal [1], essentially on the basis of: why make
it more restrictive than it needs to be? Since all we need for a minor
fault to happen is for the pages to be in the page cache, that's the
only restriction we should have.

I don't see why it shouldn't work in principle though. Imagine a
scenario where the VM guest's mapping is private, and the memory
manager's mapping is shared. I guess in this case, say for a write
from the guest:

1. The guest will generate a minor fault
2. The memory manager can modify the page via its shared mapping, and
the guest will see those changes
3. After UFFDIO_CONTINUE resolves the fault, the page is CoW-ed, and
the memory manager can no longer see the guest's version of the page

I'm not really sure *why* you'd want to do this, but it seems like it
should work.

[1]: https://patchwork.kernel.org/project/linux-mm/patch/20210107190453.3051110-2-axelrasmussen@google.com/

>
> --
> Peter Xu
>

Powered by blists - more mailing lists