On Thu, Apr 22, 2021 at 03:45:39PM -0700, Mike Kravetz wrote:
> On 3/22/21 5:50 PM, Peter Xu wrote:
> > Teach the hugetlb page fault code to understand uffd-wp special pte. For
> > example, when seeing such a pte we need to convert any write fault into a read
> > one (which is fake - we'll retry the write later if so). Meanwhile, for
> > handle_userfault() we'll need to make sure we must wait for the special swap
> > pte too just like a none pte.
> >
> > Note that we also need to teach UFFDIO_COPY about this special pte across the
> > code path so that we can safely install a new page at this special pte as long
> > as we know it's a stall entry.
> >
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > ---
> > fs/userfaultfd.c | 5 ++++-
> > mm/hugetlb.c | 34 +++++++++++++++++++++++++++-------
> > mm/userfaultfd.c | 5 ++++-
> > 3 files changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 72956f9cc892..f6fa34f58c37 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -245,8 +245,11 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
> > /*
> > * Lockless access: we're in a wait_event so it's ok if it
> > * changes under us.
> > + *
> > + * Regarding uffd-wp special case, please refer to comments in
> > + * userfaultfd_must_wait().
> > */
> > - if (huge_pte_none(pte))
> > + if (huge_pte_none(pte) || pte_swp_uffd_wp_special(pte))
> > ret = true;
> > if (!huge_pte_write(pte) && (reason & VM_UFFD_WP))
> > ret = true;
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 64e424b03774..448ef745d5ee 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -4369,7 +4369,8 @@ static inline vm_fault_t hugetlb_handle_userfault(struct vm_area_struct *vma,
> > static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> > struct vm_area_struct *vma,
> > struct address_space *mapping, pgoff_t idx,
> > - unsigned long address, pte_t *ptep, unsigned int flags)
> > + unsigned long address, pte_t *ptep,
> > + pte_t old_pte, unsigned int flags)
> > {
> > struct hstate *h = hstate_vma(vma);
> > vm_fault_t ret = VM_FAULT_SIGBUS;
> > @@ -4493,7 +4494,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> >
> > ptl = huge_pte_lock(h, mm, ptep);
> > ret = 0;
> > - if (!huge_pte_none(huge_ptep_get(ptep)))
> > + if (!pte_same(huge_ptep_get(ptep), old_pte))
> > goto backout;
> >
> > if (anon_rmap) {
> > @@ -4503,6 +4504,11 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> > page_dup_rmap(page, true);
> > new_pte = make_huge_pte(vma, page, ((vma->vm_flags & VM_WRITE)
> > && (vma->vm_flags & VM_SHARED)));
> > + if (unlikely(flags & FAULT_FLAG_UFFD_WP)) {
> > + WARN_ON_ONCE(flags & FAULT_FLAG_WRITE);
> > + /* We should have the write bit cleared already, but be safe */
> > + new_pte = huge_pte_wrprotect(huge_pte_mkuffd_wp(new_pte));
> > + }
> > set_huge_pte_at(mm, haddr, ptep, new_pte);
> >
> > hugetlb_count_add(pages_per_huge_page(h), mm);
> > @@ -4584,9 +4590,16 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> > if (unlikely(is_hugetlb_entry_migration(entry))) {
> > migration_entry_wait_huge(vma, mm, ptep);
> > return 0;
> > - } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry)))
> > + } else if (unlikely(is_hugetlb_entry_hwpoisoned(entry))) {
> > return VM_FAULT_HWPOISON_LARGE |
> > VM_FAULT_SET_HINDEX(hstate_index(h));
> > + } else if (unlikely(is_swap_special_pte(entry))) {
> > + /* Must be a uffd-wp special swap pte */
> > + WARN_ON_ONCE(!pte_swp_uffd_wp_special(entry));
> > + flags |= FAULT_FLAG_UFFD_WP;
> > + /* Emulate a read fault */
> > + flags &= ~FAULT_FLAG_WRITE;
> > + }
>
> The comment above this if/else block points out that we hold no locks
> and are only checking conditions that would cause a quick return. Yet,
> this new code is potentially modifying flags. Pretty sure we can race
> and have the entry change.
>
> Not sure of all the side effects of emulating a read if changed entry is
> not a uffd-wp special swap pte and we emulate read when we should not.
>
> Perhaps we should just put this check and modification of flags after
> taking the fault mutex and before the change below?
That's a great point. Even the WARN_ON_ONCE could trigger if the pte got
modified in parallel, so definitely broken.
Yes I'd better do that with the pgtable lock, mostly hugetlb_no_page() should
be the only function to handle this special case. So maybe I don't need to
emulate the READ fault at all, but just check pte_swp_uffd_wp_special() with
the lock, then do wrprotect properly should suffice. Maybe that's even true
for shmem, I'll think more about it.
Thanks!
--
Peter Xu
