Re: [PATCH] mm/hugetlb: avoid corrupting page->mapping in hugetlb_mcopy_atomic_pte

[Axel Rasmussen <axelrasmussen@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]

On Fri, Jul 15, 2022 at 5:35 AM Peter Xu <peterx@redhat.com> wrote:

> On Fri, Jul 15, 2022 at 11:56:40AM +0800, Miaohe Lin wrote:
> > On 2022/7/14 23:52, Peter Xu wrote:
> > > On Thu, Jul 14, 2022 at 05:59:53PM +0800, Miaohe Lin wrote:
> > >> On 2022/7/14 1:23, Andrew Morton wrote:
> > >>> On Tue, 12 Jul 2022 21:05:42 +0800 Miaohe Lin <linmiaohe@huawei.com>
> 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.
> > >>>
> > >>> Well that sounds bad.  And theories on why this has gone unnoticed
> for
> > >>> over a year?  I assume this doesn't have coverage in our selftests?
> > >>
> > >> As discussed in another thread, when minor fault handling is
> proposed, only
> > >> VM_SHARED vma is expected to be supported. And the test case is also
> missing.
> > >
> > > Yes, after this patch applied it'll be great to have the test case
> covering
> > > private mappings too.
> > >
> > > It's just that it'll be a bit more than setting test_uffdio_minor=1 for
> > > "hugetlb" test.  In hugetlb_allocate_area() we'll need to setup the
> alias
> > > too for !shared case, it'll be a bit challenging since currently we're
> > > using anonymous hugetlb mappings for private tests, and I'm not sure
> > > whether we'll need the hugetlb path back just like what we have with
> > > "hugetlb_shared" tests.
> >
> > I'm afraid not. When minor fault handling is proposed, only VM_SHARED
> vma is
> > expected to be supported. It seems it's hard to image how one might
> benefit
> > from using it with a private mapping. But I'm not sure as I'm still a
> layman
> > in userfaultfd now. Any further suggestions?
>
> IIUC so far we all think it's not required to limit it to shared mappings
> only?  The effort is mostly the same.
>
> My suggestion is above - we could enable the kselftest for it, but I don't
> strongly ask for that too because I don't know any real use of it, it'll
> still be good to have it though for completeness.  It's just that we may
> need to change some code back in 9ae8f2b849f79 on using fd-based memory, or
> I don't know how to create the alias mapping properly.
>

I agree we should either:
- Update the UFFD selftest to exercise this case
- Or, don't allow it, update vma_can_userfault() to also require VM_SHARED
for VM_UFFD_MINOR registration.

The first one is unfortunately not completely straightforward as Peter
described. I would say it's probably not worth holding up this fix while we
wait for it to happen?

I don't really have a strong preference between the two. The second option
is what I originally proposed in the first version of the minor fault
series, so going back to that isn't a problem at least from my perspective.
If in the future we find a real use case for this, we could always easily
re-enable it and add selftests for it at that point.


>
> Thanks,
>
> --
> Peter Xu
>
>

[-- Attachment #2: Type: text/html, Size: 4175 bytes --]