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

[Lance Yang <lance.yang@linux.dev>]

On Mon, Jun 15, 2026 at 01:58:15PM +0200, David Hildenbrand (Arm) wrote:
>On 6/12/26 04:48, Wei Yang wrote:
>> On Tue, May 12, 2026 at 08:55:47PM +0200, David Hildenbrand (Arm) wrote:
>>> On 5/12/26 16:35, Wei Yang wrote:
>>>>
>>>> I tried to compress above logic like this, hope it could look cleaner.

Emm ... spelling out the present/migration/device-private cases makes
this easier to review, and avoids hiding future softleaf types behind
pmd_is_valid_softleaf(), IMHO.

So I'd prefer David's explicit version[1].

>>>>
>>>> 	if (pmd_trans_huge(pmde) || pmd_is_valid_softleaf(pmde)) {
>>>> 		unsigned long pfn;
>>>> 		bool is_migration = pmd_is_migration_entry(pmde);
>>>> 		bool for_migration = !!(pvmw->flags & PVMW_MIGRATION);
>>>>
>>>> 		if (is_migration != for_migration)
>>>> 			return not_found(pvmw);
>>>>
>>>> 		if (pmd_trans_huge(pmde))
>>>> 			pfn = pmd_pfn(pmde);
>>>> 		else
>>>> 			pfn = softleaf_to_pfn(softleaf_from_pmd(pmde));
>>>>
>>>> 		if (!check_pmd(pfn, pvmw))
>>>> 			return not_found(pvmw);
>>>> 	} else if (!pmd_present(pmde)) {
>>>
>>> It's more compact, but not necessarily cleaner. In particular, I detest
>>> pmd_trans_huge(), we should phase it out.
>>>
>>> if (pmd_present(pmde) && !pmd_leaf(pmde)) {
>>> 	goto pte_table;
>>> } else if (pmd_present(pmde) || pmd_is_valid_softleaf(pmde))
>>>
>>> ...
>>>
>>> Might work as well. But once we add support for other softleaf types, we'll have
>>> to touch it again. So I'd rather just list what we actually expect.
>>>
>> 
>> Hi, David
>
>Hi,
>
>> 
>> I may not follow you. Just want to confirm whether you prefer this goes as a
>> fix first, or you prefer it goes as what you suggested here as a cleanup?
>
>I guess we should just do it properly when we're touching the code already.

+1 to that ;)

>Does that answer your question? Will you send a proper patch?

Copied the diff from [1] below, with a couple of tiny nits inline. Feel
free to grab any of this if it looks sane :)

---8<---
diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
index a4d52fdb3056..de6a255cc847 100644
--- a/mm/page_vma_mapped.c
+++ b/mm/page_vma_mapped.c
@@ -242,40 +242,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);

Could be const.

+
+                       if (!(pvmw->flags & PVMW_MIGRATION))
+                               return not_found(pvmw);
+                       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);

Ditto.
 
+                       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) &&
@@ -285,6 +273,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)))

Maybe worth a likely() here? The PMD normally shouldn't change under us.

+                       return true;
+               spin_unlock(pvmw->ptl);
+               pvmw->ptl = NULL;
+               goto restart;
+pte_table:
                if (!map_pte(pvmw, &pmde, &ptl)) {
                        if (!pvmw->pte)
---

[1] https://lore.kernel.org/linux-mm/0aab59b8-71c5-4059-8281-5dd876946528@kernel.org/

Cheers, Lance