Hi Peter,
Sorry for late reply.
On 3/22/23 12:50 AM, Peter Xu wrote:
On Tue, Mar 21, 2023 at 08:36:35PM +0100, David Hildenbrand wrote:
On 21.03.23 20:18, Peter Xu wrote:
This patch fixes an issue that a hugetlb uffd-wr-protected mapping can be writable even with uffd-wp bit set. It only happens with all these conditions met: (1) hugetlb memory (2) private mapping (3) original mapping was missing, then (4) being wr-protected (IOW, pte marker installed). Then write to the page to trigger.
Userfaultfd-wp trap for hugetlb was implemented in hugetlb_fault() before even reaching hugetlb_wp() to avoid taking more locks that userfault won't need. However there's one CoW optimization path for missing hugetlb page that can trigger hugetlb_wp() inside hugetlb_no_page(), that can bypass the userfaultfd-wp traps.
A few ways to resolve this:
(1) Skip the CoW optimization for hugetlb private mapping, considering that private mappings for hugetlb should be very rare, so it may not really be helpful to major workloads. The worst case is we only skip the optimization if userfaultfd_wp(vma)==true, because uffd-wp needs another fault anyway.
(2) Move the userfaultfd-wp handling for hugetlb from hugetlb_fault() into hugetlb_wp(). The major cons is there're a bunch of locks taken when calling hugetlb_wp(), and that will make the changeset unnecessarily complicated due to the lock operations.
(3) Carry over uffd-wp bit in hugetlb_wp(), so it'll need to fault again for uffd-wp privately mapped pages.
This patch chose option (3) which contains the minimum changeset (simplest for backport) and also make sure hugetlb_wp() itself will start to be always safe with uffd-wp ptes even if called elsewhere in the future.
This patch will be needed for v5.19+ hence copy stable.
Reported-by: Muhammad Usama Anjum usama.anjum@collabora.com Cc: linux-stable stable@vger.kernel.org Fixes: 166f3ecc0daf ("mm/hugetlb: hook page faults for uffd write protection") Signed-off-by: Peter Xu peterx@redhat.com
mm/hugetlb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 8bfd07f4c143..22337b191eae 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5478,7 +5478,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma, struct folio *pagecache_folio, spinlock_t *ptl) { const bool unshare = flags & FAULT_FLAG_UNSHARE;
- pte_t pte;
- pte_t pte, newpte; struct hstate *h = hstate_vma(vma); struct page *old_page; struct folio *new_folio;
@@ -5622,8 +5622,10 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma, mmu_notifier_invalidate_range(mm, range.start, range.end); page_remove_rmap(old_page, vma, true); hugepage_add_new_anon_rmap(new_folio, vma, haddr);
set_huge_pte_at(mm, haddr, ptep,make_huge_pte(vma, &new_folio->page, !unshare));
newpte = make_huge_pte(vma, &new_folio->page, !unshare);if (huge_pte_uffd_wp(pte))newpte = huge_pte_mkuffd_wp(newpte);set_huge_pte_at(mm, haddr, ptep, newpte);Looks correct to me. Do we have a reproducer?
I used a reproducer for the async mode I wrote (patch 2 attached, need to change to VM_PRIVATE):
https://lore.kernel.org/all/ZBNr4nohj%2FTw4Zhw@x1n/
I don't think kernel kselftest can trigger it because we don't do strict checks yet with uffd-wp bits. I've already started looking into cleanup the test cases and I do plan to add new tests to cover this.
Meanwhile, let's also wait for an ack from Muhammad. Even though the async mode is not part of the code base, it'll be a good test for verifying every single uffd-wp bit being set or cleared as expected.
I've tested by applying this patch. But the bug is still there. Just like Peter has mentioned, we are using our in progress patches related to pagemap_scan ioctl and userfaultd wp async patches to reproduce it.
To reproduce please build kernel and run pagemap_ioctl test in mm in hugetlb_mem_reproducer branch: https://gitlab.collabora.com/usama.anjum/linux-mainline/-/tree/hugetlb_mem_r...
In case you have any question on how to reproduce, please let me know. I'll try to provide a cleaner alternative.
Acked-by: David Hildenbrand david@redhat.com
Thanks,