Date:   Fri, 30 Sep 2022 14:38:01 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Hugh Dickins <hughd@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Yang Shi <shy828301@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        syzbot <syzbot+152d76c44ba142f8992b@...kaller.appspotmail.com>,
        akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, llvm@...ts.linux.dev, nathan@...nel.org,
        ndesaulniers@...gle.com, songmuchun@...edance.com,
        syzkaller-bugs@...glegroups.com, trix@...hat.com
Subject: Re: [syzbot] general protection fault in PageHeadHuge

On 09/30/22 12:05, Peter Xu wrote:
> On Thu, Sep 29, 2022 at 04:33:53PM -0700, Mike Kravetz wrote:
> > I was able to do a little more debugging:
> > 
> > As you know the hugetlb calling path to handle_userfault is something
> > like this,
> > 
> > hugetlb_fault
> > 	mutex_lock(&hugetlb_fault_mutex_table[hash]);
> > 	ptep = huge_pte_alloc(mm, vma, haddr, huge_page_size(h));
> > 	if (huge_pte_none_mostly())
> > 		hugetlb_no_page()
> > 			page = find_lock_page(mapping, idx);
> > 			if (!page) {
> > 				if (userfaultfd_missing(vma))
> > 					mutex_unlock(&hugetlb_fault_mutex_table[hash]);
> > 					return handle_userfault()
> > 			}
> > 
> > For anon mappings, find_lock_page() will never find a page, so as long
> > as huge_pte_none_mostly() is true we will call into handle_userfault().
> > 
> > Since your analysis shows the testcase should never call handle_userfault() for
> > a write fault, I simply added a 'if (flags & FAULT_FLAG_WRITE) printk' before
> > the call to handle_userfault().  Sure enough, I saw plenty of printk messages.
> > 
> > Then, before calling handle_userfault() I added code to take the page table
> > lock and test huge_pte_none_mostly() again.  In every FAULT_FLAG_WRITE case,
> > this second test of huge_pte_none_mostly() was false.  So, the condition
> > changed from the check in hugetlb_fault until the check (with page table
> > lock) in hugetlb_no_page.
> > 
> > IIUC, the only code that should be modifying the pte in this test is
> > hugetlb_mcopy_atomic_pte().  It also holds the hugetlb_fault_mutex while
> > updating the pte.
> > 
> > It 'appears' that hugetlb_fault is not seeing the updated pte and I can
> > only guess that it is due to some caching issues.
> > 
> > After writing the pte in hugetlb_mcopy_atomic_pte() there is this comment.
> > 
> > 	/* No need to invalidate - it was non-present before */
> > 	update_mmu_cache(dst_vma, dst_addr, dst_pte);
> > 
> > I suspect that is true.  However, it seems like this test depends on all
> > CPUs seeing the updated pte immediately?
> > 
> > I added some TLB flushing to hugetlb_mcopy_atomic_pte, but it did not make
> > any difference.  Suggestions would be appreciated as cache/tlb/???  flushing
> > issues take me a while to figure out.
> 
> This morning when I went back and rethink the matter, I just found that the
> common hugetlb path handles private anonymous mappings with all empty page
> cache as you explained above.  In that sense the two patches I posted may
> not really make sense even if they can pass the tests.. and maybe that's
> also the reason why the reservations got messed up.  This is also something
> I found after I read more on the reservation code e.g. no matter private or
> shared hugetlb mappings we only reserve that only number of pages when mmap().
> 
> Indeed if with that in mind the UFFDIO_COPY should also work because
> hugetlb fault handler checks pte first before page cache, so uffd missing
> should still work as expected.
> 
> It makes sense especially for hugetlb to do that otherwise there can be
> plenty of zero huge pages cached in the page cache.  I'm not sure whether
> this is the reason hugetlb does it differently (e.g. comparing to shmem?),
> it'll be great if I can get a confirmation.  If it's true please ignore the
> two patches I posted.
> 
> I think what you analyzed is correct in that the pte shouldn't go away
> after being armed once.  One more thing I tried (actually yesterday) was
> SIGBUS the process when the write missing event was generated, and I can
> see the user stack points to the cmpxchg() of the pthread_mutex_lock().  It
> means indeed it moved forward and passed the mutex type check, it also
> means it should have seen a !none pte already with at least reading
> permission, in that sense it matches with "missing TLB" possibility
> experiment mentioned above, because for a missing TLB it should keep
> stucking at the read not write.  It's still uncertain why the pte can go
> away somehow from under us and why it quickly re-appears according to your
> experiment.
> 

I 'think' it is more of a race with all cpus seeing the pte update.  To be
honest, I can not wrap my head around how that can happen.

I did not really like your idea of adding anon (or private) pages to the
page cache.  As you discovered, there is code like reservations which depend
on current behavior.

It seems to me that for 'missing' hugetlb faults there are two specific cases:
1) Shared or file backed mappings.  In this case, the page cache is the
   'source of truth'.  If there is not a page in the page cache, then we
   hand off to userfault with VM_UFFD_MISSING.
2) anon or private mappings.  In this case, pages are not in the page cache.
   The page table is the 'source of truth'.  Early in hugetlb fault processing
   we check the page table (huge_pte_none_mostly).  However, as my debug code
   has shown, checking the page table again with lock held will reveal that
   the pte has in fact been updated.

My thought was that regular anon pages would have the same issue.  So, I looked
at the calling code there.  In do_anonymous_page() there is this block:

	/* Use the zero-page for reads */
	if (!(vmf->flags & FAULT_FLAG_WRITE) &&
			!mm_forbids_zeropage(vma->vm_mm)) {
		entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address),
						vma->vm_page_prot));
		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
				vmf->address, &vmf->ptl);
		if (!pte_none(*vmf->pte)) {
			update_mmu_tlb(vma, vmf->address, vmf->pte);
			goto unlock;
		}
		ret = check_stable_address_space(vma->vm_mm);
		if (ret)
			goto unlock;
		/* Deliver the page fault to userland, check inside PT lock */
		if (userfaultfd_missing(vma)) {
			pte_unmap_unlock(vmf->pte, vmf->ptl);
			return handle_userfault(vmf, VM_UFFD_MISSING);
		}
		goto setpte;
	}

Notice that here the pte is checked while holding the page table lock while
to make the decision to call handle_userfault().

In my testing, if we check huge_pte_none_mostly() while holding the page table
lock before calling handle_userfault we will not experience the failure.  Can
you see if this also resolves the issue in your environment?  I do not love
this solution as I still can not explain how this code is missing the pte
update.

>From f910e7155d6831514165af35e0d75574124a4477 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <mike.kravetz@...cle.com>
Date: Fri, 30 Sep 2022 13:45:08 -0700
Subject: [PATCH] hugetlb: check pte with page table lock before handing to
 userfault

In hugetlb_no_page we decide a page is missing if not present in the
page cache.  This is perfectly valid for shared/file mappings where
pages must exist in the page cache.  For anon/private mappings, the page
table must be checked.  This is done early in hugetlb_fault processing
and is the reason we enter hugetlb_no_page.  However, the early check is
made without holding the page table lock.  There could be racing updates
to the pte entry, so check again with page table lock held before
deciding to call handle_userfault.

Signed-off-by: Mike Kravetz <mike.kravetz@...cle.com>
---
 mm/hugetlb.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 60e077ce6ca7..4cb44a4629b8 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5560,10 +5560,29 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 		if (idx >= size)
 			goto out;
 		/* Check for page in userfault range */
-		if (userfaultfd_missing(vma))
+		if (userfaultfd_missing(vma)) {
+			/*
+			 * For missing pages, the page cache (checked above) is
+			 * the 'source of truth' for shared mappings.  For anon
+			 * mappings, the source of truth is the page table.  We
+			 * already checked huge_pte_none_mostly() in
+			 * hugetlb_fault.  However, there could be racing
+			 * updates.  Check again while holding page table lock
+			 * before handing off to userfault.
+			 */
+			if (!(vma->vm_flags & VM_MAYSHARE)) {
+				ptl = huge_pte_lock(h, mm, ptep);
+				if (!huge_pte_none_mostly(huge_ptep_get(ptep))) {
+					spin_unlock(ptl);
+					ret = 0;
+					goto out;
+				}
+				spin_unlock(ptl);
+			}
 			return hugetlb_handle_userfault(vma, mapping, idx,
 						       flags, haddr, address,
 						       VM_UFFD_MISSING);
+		}
 
 		page = alloc_huge_page(vma, haddr, 0);
 		if (IS_ERR(page)) {
-- 
2.37.3

Powered by blists - more mailing lists