Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Sep 2022 12:05:11 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Mike Kravetz <mike.kravetz@oracle.com>
Cc:     Hugh Dickins <hughd@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Yang Shi <shy828301@gmail.com>,
        Matthew Wilcox <willy@infradead.org>,
        syzbot <syzbot+152d76c44ba142f8992b@syzkaller.appspotmail.com>,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, llvm@lists.linux.dev, nathan@kernel.org,
        ndesaulniers@google.com, songmuchun@bytedance.com,
        syzkaller-bugs@googlegroups.com, trix@redhat.com
Subject: Re: [syzbot] general protection fault in PageHeadHuge
Message-ID: <YzcTt3P3ofvbGQmi@x1n>
References: <0000000000006c300705e95a59db@google.com>
 <Yy4g/BKpnJga1toG@monkey>
 <7693a84-bdc2-27b5-2695-d0fe8566571f@google.com>
 <Yy5WHvioerD2Sev1@x1n>
 <Yy8dBrmvDmqlsRXE@x1n>
 <Yy9T/JmVb4ymf6WM@monkey>
 <YzDuHbuo2x/b2Mbr@x1n>
 <YzMjxY5O6Hf/IPTx@monkey>
 <YzMomT+OusJnLOPC@x1n>
 <YzYrYVeA0b9d5dos@monkey>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YzYrYVeA0b9d5dos@monkey>
Precedence: bulk

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.

-- 
Peter Xu