Date:   Sun, 30 Oct 2022 17:29:19 -0400
From:   Peter Xu <peterx@...hat.com>
To:     linux-kernel@...r.kernel.org, linux-mm@...ck.org
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        James Houghton <jthoughton@...gle.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        David Hildenbrand <david@...hat.com>,
        Muchun Song <songmuchun@...edance.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Mike Kravetz <mike.kravetz@...cle.com>, peterx@...hat.com,
        Rik van Riel <riel@...riel.com>
Subject: [PATCH RFC 00/10] mm/hugetlb: Make huge_pte_offset() thread-safe for pmd unshare

This can be seen as a follow-up series to Mike's recent hugetlb vma lock
series for pmd unsharing.  So this series also depends on that one.  But
there're some huge_pte_offset() paths that seem to be still racy on pmd
unsharing (as they don't take vma lock), more below.

Hopefully this series can make it a more complete resolution for pmd
unsharing.

Problem
=======

huge_pte_offset() is a major helper used by hugetlb code paths to walk a
hugetlb pgtable.  It's used mostly everywhere since that's needed even
before taking the pgtable lock.

huge_pte_offset() is always called with mmap lock held with either read or
write.

For normal memory types that's far enough, since any pgtable removal
requires mmap write lock (e.g. munmap or mm destructions).  However hugetlb
has the pmd unshare feature, it means not only the pgtable page can be gone
from under us when we're doing a walking, but also the pgtable page we're
walking (even after unshared, in this case it can only be the huge PUD page
which contains 512 huge pmd entries, with the vma VM_SHARED mapped).  It's
possible because even though freeing the pgtable page requires mmap write
lock, it doesn't help us from when we're walking on another mm's pgtable,
so it's still on risk even if we're with the current->mm's mmap lock.

The recent work from Mike on vma lock can resolve most of this already.
It's achieved by forbidden pmd unsharing during the lock being taken, so no
further risk of the pgtable page being freed.

But it means it'll work only if we take the vma lock for all the places
around huge_pte_offset().  There're already a bunch of them that we did as
per the latest mm-unstable, but also a lot that we didn't for various
reasons.  E.g. it may not be applicable for not-allow-to-sleep contexts
like FOLL_NOWAIT.

I have totally no report showing that I can trigger such a race, but from
code wise I never see anything that stops the race from happening.  This
series is trying to resolve that problem.

Resolution
==========

What this patch proposed is, besides using the vma lock, we can also use
RCU to protect the pgtable page from being freed from under us when
huge_pte_offset() is used.  The idea is kind of similar to RCU fast-gup.
Note that fast-gup is very safe regarding pmd unsharing even before vma
lock, because fast-gup relies on RCU to protect walking any pgtable page,
including another mm's.

To apply the same idea to huge_pte_offset(), it means with proper RCU
protection the pte_t* pointer returned from huge_pte_offset() can also be
always safe to access and de-reference, along with the pgtable lock that
was bound to the pgtable page.

Patch Layout
============

Patch 1 is a trivial cleanup that I noticed when working on this.  Please
shoot if anyone think I should just post it separately, or hopefully I can
still just carry it over.

Patch 2 is the gut of the patchset, describing how we should use the helper
huge_pte_offset() correctly. Only a comment patch but should be the most
important one, as the follow up patches are just trying to follow the rule
it setup here.

The rest patches resolve all the call sites of huge_pte_offset() to make
sure either it's with the vma lock (which is perfectly good enough for
safety in this case; the last patch commented on all those callers to make
sure we won't miss a single case, and why they're safe).  Besides, each of
the patch will add rcu protection to one caller of huge_pte_offset().

Tests
=====

Only lightly tested on hugetlb kselftests including uffd, no more errors
triggered than current mm-unstable (hugetlb-madvise fails before/after
here, with error "Unexpected number of free huge pages line 207"; haven't
really got time to look into it).

Since this is so far only discussed with Mike quickly in the other thread,
marking this as RFC for now as I could have missed something.

Comments welcomed, thanks.

Peter Xu (10):
  mm/hugetlb: Let vma_offset_start() to return start
  mm/hugetlb: Comment huge_pte_offset() for its locking requirements
  mm/hugetlb: Make hugetlb_vma_maps_page() RCU-safe
  mm/hugetlb: Make userfaultfd_huge_must_wait() RCU-safe
  mm/hugetlb: Make walk_hugetlb_range() RCU-safe
  mm/hugetlb: Make page_vma_mapped_walk() RCU-safe
  mm/hugetlb: Make hugetlb_follow_page_mask() RCU-safe
  mm/hugetlb: Make follow_hugetlb_page RCU-safe
  mm/hugetlb: Make hugetlb_fault() RCU-safe
  mm/hugetlb: Comment at rest huge_pte_offset() places

 arch/arm64/mm/hugetlbpage.c | 32 ++++++++++++++++++++++++++
 fs/hugetlbfs/inode.c        | 39 ++++++++++++++++++--------------
 fs/userfaultfd.c            |  4 ++++
 include/linux/rmap.h        |  3 +++
 mm/hugetlb.c                | 45 +++++++++++++++++++++++++++++++++++--
 mm/page_vma_mapped.c        |  7 +++++-
 mm/pagewalk.c               |  5 +++++
 7 files changed, 115 insertions(+), 20 deletions(-)

-- 
2.37.3

Powered by blists - more mailing lists