Re: [syzbot] general protection fault in PageHeadHuge

[Mike Kravetz]

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.