* Re: [Patch v2] mm/page_vma_mapped: revalidate and do proper check before return device-private pmd [not found] <20260616063436.20455-1-richard.weiyang@gmail.com> @ 2026-06-16 12:30 ` Lance Yang 2026-06-16 23:50 ` Wei Yang 0 siblings, 1 reply; 2+ messages in thread From: Lance Yang @ 2026-06-16 12:30 UTC (permalink / raw) To: richard.weiyang Cc: akpm, david, ljs, riel, liam, vbabka, harry, jannh, balbirs, ziy, sj, linux-mm, lorenzo.stoakes, stable, Lance Yang 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? >+ if (!check_pmd(softleaf_to_pfn(entry), pvmw)) >+ return not_found(pvmw); >+ } else if (pmd_is_device_private_entry(pmde)) { >+ softleaf_t entry = softleaf_from_pmd(pmde); > >+ if (pvmw->flags & PVMW_MIGRATION) >+ return not_found(pvmw); >+ if (!check_pmd(softleaf_to_pfn(entry), pvmw)) >+ return not_found(pvmw); >+ } else { > if ((pvmw->flags & PVMW_SYNC) && > thp_vma_suitable_order(vma, pvmw->address, > PMD_ORDER) && >@@ -286,6 +274,15 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) > step_forward(pvmw, PMD_SIZE); > continue; > } >+ >+ /* Double-check under PTL that the PMD didn't change. */ >+ pvmw->ptl = pmd_lock(mm, pvmw->pmd); >+ if (pmd_same(pmde, pmdp_get(pvmw->pmd))) >+ return true; >+ spin_unlock(pvmw->ptl); >+ pvmw->ptl = NULL; >+ goto restart; >+pte_table: > if (!map_pte(pvmw, &pmde, &ptl)) { > if (!pvmw->pte) > goto restart; >-- >2.34.1 > Cheers, Lance ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Patch v2] mm/page_vma_mapped: revalidate and do proper check before return device-private pmd 2026-06-16 12:30 ` [Patch v2] mm/page_vma_mapped: revalidate and do proper check before return device-private pmd Lance Yang @ 2026-06-16 23:50 ` Wei Yang 0 siblings, 0 replies; 2+ messages in thread From: Wei Yang @ 2026-06-16 23:50 UTC (permalink / raw) To: Lance Yang Cc: richard.weiyang, akpm, david, ljs, riel, liam, vbabka, harry, jannh, balbirs, ziy, sj, linux-mm, lorenzo.stoakes, stable On Tue, Jun 16, 2026 at 08:30:01PM +0800, 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 :) > Hi, Lance Thanks for take a look. I am trying to understand the scenario you mentioned. Let's say A migrate a pmd and B want to unmap the pmd. A B try to migrate a pmd pmd is set to migration entry unmap the pmd ... managed to finish migration ...still see migration entry, so skipped and unmap fail Would this be a timing case? Even B grab the PTL, it still could see migration entry if B visit pmd before A finish migration. Maybe I miss something, look forward your insight. >Not sure about practical fallout, but should these PMD-level not_found() >cases also take PTL and restart if PMD changed? > -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-06-16 23:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260616063436.20455-1-richard.weiyang@gmail.com>
2026-06-16 12:30 ` [Patch v2] mm/page_vma_mapped: revalidate and do proper check before return device-private pmd Lance Yang
2026-06-16 23:50 ` Wei Yang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.