On 27 Apr 2024, at 0:25, John Hubbard wrote: > On 4/26/24 7:53 AM, Zi Yan wrote: > > Hi Zi (and Ryan)! > >>>>>> lockless pgtable walker could see the migration entry pmd in this state >>>>>> and start interpretting the fields as if it were present, leading to >>>>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable walker. >>>>> >>>>> Could you please explain how bad things might happen ? >>>> >>>> See 2 places where pmdp_get_lockless() is called in gup.c, without the PTL. >>>> These could both return the swap pte for which pmd_mkinvalid() has been called. >>>> In both cases, this would lead to the pmd_present() check eroneously returning >>>> true, eventually causing incorrect interpretation of the pte fields. e.g.: >>>> >>>> gup_pmd_range() >>>> pmd_t pmd = pmdp_get_lockless(pmdp); >>>> gup_huge_pmd(pmd, ...) >>>> page = nth_page(pmd_page(orig), (addr & ~PMD_MASK) >> PAGE_SHIFT); >>>> >>>> page is guff. >>>> >>>> Let me know what you think! >> >> Add JohnH to check GUP code. > > Ryan is correct about this behavior. > > By the way, remember that gup is not the only lockless page table > walker: there is also the CPU hardware itself, which inconveniently > refuses to bother with taking page table locks. :) > > So if we have code that can make a non-present PTE appear to be present > to any of these page walkers, whether software or hardware, it's a > definitely Not Good and will lead directly to bugs. This issue does not bother hardware, because the PTE_VALID/PMD_SECT_VALID is always unset and hardware always sees this PMD as invalid. It is a pure software issue, since for THP splitting, we do not want hardware to access the page but still allow kernel to user pmd_page() to get the pfn, so pmd_present() returns true even if PTE_VALID/PMD_SECT_VALID is unset by setting and checking PMD_PRESENT_INVALID bit. pmd_mkinvalid() sets PMD_PRESENT_INVALID, turning a migration entry from !pmd_present() to pmd_present(), while it is always a invalid PMD to hardware. > > Since I had to study this patch and discussion a bit in order to > respond, I'll go ahead and also reply to the original patch with review > comments. > > > thanks, > -- > John Hubbard > NVIDIA -- Best Regards, Yan, Zi
Attachment:
signature.asc
Description: OpenPGP digital signature
