On Sun, Oct 02, 2022 at 06:16:53PM -0700, Mike Kravetz wrote:
> On 09/30/22 23:01, Peter Xu wrote:
> > On Fri, Sep 30, 2022 at 10:47:45PM -0400, Peter Xu wrote:
> > From fe9e50551f3fdb7107315784affca4f9b1c4720f Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@xxxxxxxxxx>
> > Date: Fri, 30 Sep 2022 22:22:44 -0400
> > Subject: [PATCH] mm/hugetlb: Fix race condition of uffd missing handling
> > Content-type: text/plain
> >
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > ---
> > mm/hugetlb.c | 36 +++++++++++++++++++++++++++++++++---
> > 1 file changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index dd29cba46e9e..5015d8aa5da4 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -5557,9 +5557,39 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> > if (!page) {
> > /* Check for page in userfault range */
> > if (userfaultfd_missing(vma)) {
> > - ret = hugetlb_handle_userfault(vma, mapping, idx,
> > - flags, haddr, address,
> > - VM_UFFD_MISSING);
> > + bool same;
> > +
> > + /*
> > + * Since hugetlb_no_page() was examining pte
> > + * without pgtable lock, we need to re-test under
> > + * lock because the pte may not be stable and could
> > + * have changed from under us. Try to detect
> > + * either changed or during-changing ptes and bail
> > + * out properly.
> > + *
> > + * One example of changing pte is in-progress CoW
> > + * of private mapping, which will clear+flush pte
> > + * then reinstall the new one.
> > + *
> > + * Note that userfaultfd is actually fine with
> > + * false positives (e.g. caused by pte changed),
> > + * but not wrong logical events (e.g. caused by
> > + * reading a pte during changing). The latter can
> > + * confuse the userspace, so the strictness is very
> > + * much preferred. E.g., MISSING event should
> > + * never happen on the page after UFFDIO_COPY has
> > + * correctly installed the page and returned.
> > + */
>
> Thanks Peter!
>
> The wording and pte_same check here is better than what I proposed. I think
> that last paragraph above should go into the commit message as it describes
> user visible effects (missing event after UFFDIO_COPY has correctly installed
> the page and returned).
Will do.
>
> This seems to have existed since hugetlb userfault support was added. It just
> became exposed recently due to locking changes going into 6.1. However, I
> think it may have existed in the window after hugetlb userfault support was
> added and before current i_mmap_sema locking for pmd sharing was added.
Agreed.
> Just a long way of saying I am not sure cc stable if of much value.
Logically the change is stable material. I had worry that after uffd-wp
intergration with hugetlb it's indeed possible to trigger on the CoWs we're
encountering already, so IMO still something good to have for 5.19.
I just saw that you proposed a similar fix in 4643d67e8cb0b35 on a similar
page migration race three years ago. I'm not sure whether it also can
happen with uffd missing modes too even before uffd-wp introduced.
I think I'll first post the patch with Fixes attached without having stable
tagged, but let me know your thoughts. No worry on the backport, I can
take care of doing that and tests.
I also plan to add your co-devel tag if you're fine with it because this
patch is a collaboration effort IMO, but please let me know either here or
directly replying to the patch if it's posted if you think that's inproper
in any form.
Thanks Mike!
--
Peter Xu
