Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1568994684-1425-1-git-send-email-hqjagain@gmail.com>
 <1a162778-41b9-4428-1058-82aaf82314b1@nvidia.com> <CAJRQjodUajhYgQV7Z821qFwYzR0jSxJt54y=4XjqYW68mNMzTQ@mail.gmail.com>
 <ce863f5b-2337-1ac3-4d3d-d1d62acbba24@nvidia.com>
In-Reply-To: <ce863f5b-2337-1ac3-4d3d-d1d62acbba24@nvidia.com>
From:   Qiujun Huang <hqjagain@gmail.com>
Date:   Sat, 21 Sep 2019 09:47:42 +0800
Message-ID: <CAJRQjofBLyXCKXzS7jxs3FjOyh3YydiCAsT1PmyaqvvRhWxYiQ@mail.gmail.com>
Subject: Re: [PATCH 3/3] mm:fix gup_pud_range
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     akpm@linux-foundation.org, ira.weiny@intel.com, jgg@ziepe.ca,
        dan.j.williams@intel.com, rppt@linux.ibm.com,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
        keith.busch@intel.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Sep 21, 2019 at 9:19 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 9/20/19 5:33 PM, Qiujun Huang wrote:
> >> On 9/20/19 8:51 AM, Qiujun Huang wrote:
> ...
> >> It would be nice if this spelled out a little more clearly what's
> >> wrong. I think you and Aneesh are saying that the entry is really
> >> a swap entry, created by the MCE response to a bad page?
> > do_machine_check->
> > do_memory_failure->
> > memory_failure->
> > hwpoison_user_mappings
> > will updated PUD level PTE entry as a swap entry.
> >
> > static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> > unsigned long address, void *arg)
> > {
> > ...
> > if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
> > pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
>
> OK, that helps. Let's add something approximately like this to the
> commit description:
>
> do_machine_check()
>   do_memory_failure()
>     memory_failure()
>       hw_poison_user_mappings()
>         try_to_unmap()
>           pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
>
> ...and now we have a swap entry that indicates that the page entry
> refers to a bad (and poisoned) page of memory, but gup_fast() at this
> level of the page table was ignoring swap entries, and incorrectly
> assuming that "!pxd_none() == valid and present".
>
> And this was not just a poisoned page problem, but a generaly swap entry
> problem. So, any swap entry type (device memory migration, numa migration,
> or just regular swapping) could lead to the same problem.
>
> Fix this by checking for pxd_present(), instead of pxd_none().
>
>
> > ...
> >>
> >>>
> >>> Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
> >>> ---
> >>>  mm/gup.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/mm/gup.c b/mm/gup.c
> >>> index 98f13ab..6157ed9 100644
> >>> --- a/mm/gup.c
> >>> +++ b/mm/gup.c
> >>> @@ -2230,6 +2230,8 @@ static int gup_pud_range(p4d_t p4d, unsigned long addr, unsigned long end,
> >>>               next = pud_addr_end(addr, end);
> >>>               if (pud_none(pud))
> >>>                       return 0;
> >>> +             if (unlikely(!pud_present(pud)))
> >>> +                     return 0;
> >>
> >> If the MCE hwpoison behavior puts in swap entries, then it seems like all
> >> page table walkers would need to check for p*d_present(), and maybe at all
> >> levels too, right?
> > I think so
> >>
>
> Should those changes be part of this fix, do you think?

Yes, please.Thanks
>
> thanks,
> --
> John Hubbard
> NVIDIA