On Thu, Mar 23, 2023 at 08:33:07PM +0500, Muhammad Usama Anjum wrote: > 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 > >>> Cc: linux-stable > >>> Fixes: 166f3ecc0daf ("mm/hugetlb: hook page faults for uffd write protection") > >>> Signed-off-by: Peter Xu > >>> --- > >>> 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); > >>> folio_set_hugetlb_migratable(new_folio); > >>> /* Make the old page be freed below */ > >>> new_folio = page_folio(old_page); > >> > >> 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_reproducer > > In case you have any question on how to reproduce, please let me know. I'll > try to provide a cleaner alternative. Hmm, I think my current fix is incomplete if not wrong. The root cause should still be valid, however I overlooked another path: if (page_mapcount(old_page) == 1 && PageAnon(old_page)) { if (!PageAnonExclusive(old_page)) page_move_anon_rmap(old_page, vma); if (likely(!unshare)) set_huge_ptep_writable(vma, haddr, ptep); delayacct_wpcopy_end(); return 0; } We should bail out early in this path, and it'll be even easier we always bail out hugetlb_wp() as long as uffd-wp is detected because userfault should always be handled before any decision to CoW. v2 attached.. Please give it another shot. Thanks, -- Peter Xu