Re: [Patch v2] mm/page_vma_mapped: revalidate and do proper check before return device-private pmd

[Balbir Singh <balbirs@nvidia.com>]

On Tue, Jun 16, 2026 at 03:07:53PM +0200, David Hildenbrand (Arm) wrote:
> On 6/16/26 14:30, Lance Yang wrote:
> > 
> > On Tue, Jun 16, 2026 at 06:34:36AM +0000, Wei Yang wrote:
> > [...]
> >> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> >> index 2ccbabfb2cc1..21635fab209c 100644
> >> --- a/mm/page_vma_mapped.c
> >> +++ b/mm/page_vma_mapped.c
> >> @@ -243,40 +243,28 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> >> 		 */
> >> 		pmde = pmdp_get_lockless(pvmw->pmd);
> >>
> >> -		if (pmd_trans_huge(pmde) || pmd_is_migration_entry(pmde)) {
> >> -			pvmw->ptl = pmd_lock(mm, pvmw->pmd);
> >> -			pmde = *pvmw->pmd;
> >> -			if (!pmd_present(pmde)) {
> >> -				softleaf_t entry;
> >> -
> >> -				if (!thp_migration_supported() ||
> >> -				    !(pvmw->flags & PVMW_MIGRATION))
> >> -					return not_found(pvmw);
> >> -				entry = softleaf_from_pmd(pmde);
> >> -
> >> -				if (!softleaf_is_migration(entry) ||
> >> -				    !check_pmd(softleaf_to_pfn(entry), pvmw))
> >> -					return not_found(pvmw);
> >> -				return true;
> >> -			}
> >> -			if (likely(pmd_trans_huge(pmde))) {
> >> -				if (pvmw->flags & PVMW_MIGRATION)
> >> -					return not_found(pvmw);
> >> -				if (!check_pmd(pmd_pfn(pmde), pvmw))
> >> -					return not_found(pvmw);
> >> -				return true;
> >> -			}
> >> -			/* THP pmd was split under us: handle on pte level */
> >> -			spin_unlock(pvmw->ptl);
> >> -			pvmw->ptl = NULL;
> >> -		} else if (!pmd_present(pmde)) {
> >> -			const softleaf_t entry = softleaf_from_pmd(pmde);
> >> -
> >> -			if (softleaf_is_device_private(entry)) {
> >> -				pvmw->ptl = pmd_lock(mm, pvmw->pmd);
> >> -				return true;
> >> -			}
> >> +		if (pmd_present(pmde)) {
> >> +			if (!pmd_leaf(pmde))
> >> +				goto pte_table;
> >> +			if (pvmw->flags & PVMW_MIGRATION)
> >> +				return not_found(pvmw);
> >> +			if (!check_pmd(pmd_pfn(pmde), pvmw))
> >> +				return not_found(pvmw);
> >> +		} else if (pmd_is_migration_entry(pmde)) {
> >> +			softleaf_t entry = softleaf_from_pmd(pmde);
> >> +
> >> +			if (!(pvmw->flags & PVMW_MIGRATION))
> >> +				return not_found(pvmw);
> > 
> > Looked at history a bit, and I wonder if this changed something old
> > here ...
> > 
> > Since 616b8371539a ("mm: thp: enable thp migration in generic path"), PMD
> > migration handling took PTL before doing PVMW_MIGRATION/PFN checks,
> > including not_found() cases. So lockless PMD read was just a filter ...
> > 
> > With this fix, true case gets final pmd_same() check, but this
> > not_found() case happens before taking PTL.
> > 
> > So a !PVMW_MIGRATION walker could race with someone, e.g.
> > remove_migration_pmd(): we make the not_found() decision from old PMD
> > value that still says "migration", while real *pvmw->pmd may already be
> > present again. We return without ever taking PTL :)
> > 
> > Not sure about practical fallout, but should these PMD-level not_found()
> > cases also take PTL and restart if PMD changed?
> I was hoping that we could so something similar to the PTE case.
> 
> In map_pte(), we check whether the PMD changed, which is slightly different.
> 
> The actual check happens in check_pte() after grabbing the PTL.
> 
> For the case you describe, map_pte() would find !pte_none(ptent) ...
> !pte_present(ptent) and !is_migration, and effectively grab the lock and proceed
> to check_pte().
> 
> In check_pte() we re-check under lock indeed.
>

Thinking of the practical fallout, not finding the PMD for a non
migration worker should be OK. Is there a case where it's not OK to
report the old state.

Balbir