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

[Wei Yang]

On Mon, Jun 22, 2026 at 02:46:40PM +0100, Lorenzo Stoakes wrote:
>+cc Lance, linux-kernel
>
>Your subject line is 83 characters long and is way too detailed how about 'fix
>device-private PMD handling'?
>

Got it.

>You forgot to include linux-kernel@xxxxxxxxxxxxxxx on the mail, lore seems to be
>a bit broken atm but in general it's helpful to include that.

Got it.

So usually we send a patch to both linux-mm and linux-kernel? If so, I
remember is later actions.

>
>Also is useful to make this [PATCH mm-hotfixes] to make it really clear it's
>intended as a hotfix.
>

Got it.

>Some commit msg language nits:
>
>On Mon, Jun 22, 2026 at 01:06:51PM +0000, Wei Yang wrote:
>> For pmd_trans_huge() and pmd_is_migration_entry(), we does following
>> before return the pmd entry:
>
>Sounds better as:
>
>	For PMD entries that satisfy pmd_trans_huge() or pmd_is_migration_entry(), we
>	perform the following actions:
>

Sure.

>>
>>   * re-validate pmd entry after PTL
>>   * check PVMW_MIGRATION
>>   * check_pmd()
>>   * handle on pte level if split under us
>>
>> But for device-private pmd, we just return after pmd_lock().
>
>->
>
>	However, for device-private PMD entries, we simply acquire the PMD lock
>	and return.
>

Sure.

>Also can you please give some justification here as to why all this also applies
>to device-private PMD? Right now it sounds hand wavey.
>

I thought below paragraph explain it. Not sure what justification is preferred.

>> If a softleaf entry is present, e.g. device-private pmd, the existing
>> code simply acquires the PMD lock and returns success even if
>> PVMW_MIGRATION is set (indicating a migration entry is sought), meaning
>> that the caller can incorrectly interpret the entry as something it is
>> not, causing data corruption.
>
>This is repetitive, you already mentioned device-private PMD, you already
>mentioned that it simply acquires the PMD lock.
>

Ah, I copied your suggestion from [1]. Hope I don't misunderstand it.

[1]: https://lore.kernel.org/linux-mm/ajUXNjRMraKb6k2n@lucifer/

>You should talk about what issue it caused and why:
>
>	This is particularly problematic when PVMW_MIGRATION is set (meaning a
>	migration entry is sought), as it causes a device-private PMD entry to
>	be returned with a different data layout, causing memory corruption.
>

This looks good. I would take this one, if you prefer.

>>
>> This patch fixes commit 65edfda6f3f2 ("mm/rmap: extend rmap and migration
>> support device-private entries") by following the same pattern as
>> pmd_trans_huge() and pmd_is_migration_entry() for device private entry.
>
>This is pretty useless. We see what patch it fixes in the Fixes tag, and you're
>just repeating things you said above, I'd drop it.
>

Got it.

>> Fixes: 65edfda6f3f2 ("mm/rmap: extend rmap and migration support device-private entries")
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: Wei Yang <richard.weiyang@xxxxxxxxx>
>> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: Balbir Singh <balbirs@xxxxxxxxxx>
>> Cc: SeongJae Park <sj@xxxxxxxxxx>
>> Cc: Zi Yan <ziy@xxxxxxxxxx>
>> Cc: Lorenzo Stoakes <ljs@xxxxxxxxxx>
>>
>> ---
>> v3:
>>   * remove cleanup part, only fix the issue for device-private entry
>>   * refine user effect description based on Lorenzo's suggestion
>> v2: https://lore.kernel.org/all/20260616063436.20455-1-richard.weiyang@xxxxxxxxx/T/#u
>>   * specify the possible error case of current code and user visible effect
>>   * besides fix, cleanup the pmd entry handling based on David's suggestion
>>
>> v1: https://lore.kernel.org/linux-mm/20260508013728.21285-1-richard.weiyang@xxxxxxxxx/
>> ---
>>  mm/page_vma_mapped.c | 32 ++++++++++++++++++++++----------
>>  1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>> index 2ccbabfb2cc1..8de3c6b82df6 100644
>> --- a/mm/page_vma_mapped.c
>> +++ b/mm/page_vma_mapped.c
>> @@ -270,21 +270,33 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
>>  			spin_unlock(pvmw->ptl);
>>  			pvmw->ptl = NULL;
>>  		} else if (!pmd_present(pmde)) {
>> -			const softleaf_t entry = softleaf_from_pmd(pmde);
>> +			softleaf_t entry = softleaf_from_pmd(pmde);
>>
>>  			if (softleaf_is_device_private(entry)) {
>>  				pvmw->ptl = pmd_lock(mm, pvmw->pmd);
>> -				return true;
>> -			}
>>
>> -			if ((pvmw->flags & PVMW_SYNC) &&
>> -			    thp_vma_suitable_order(vma, pvmw->address,
>> -						   PMD_ORDER) &&
>> -			    (pvmw->nr_pages >= HPAGE_PMD_NR))
>> -				sync_with_folio_pmd_zap(mm, pvmw->pmd);
>> +				entry = softleaf_from_pmd(*pvmw->pmd);
>>
>> -			step_forward(pvmw, PMD_SIZE);
>> -			continue;
>> +				if (softleaf_is_device_private(entry)) {
>
>This is all very horrible. You have an example of how pmde is re-got in the
>pmd_trans_huge() branch and pmd_is_device_private_entry() exists...
>
>We can just make this another branch and do the re-check more neatly.
>

I plan to keep the change small, but yeah it is ugly.

>I enclose a patch that does that (untested, please check).
>
>
>> +					if (pvmw->flags & PVMW_MIGRATION)
>> +						return not_found(pvmw);
>> +					if (!check_pmd(softleaf_to_pfn(entry), pvmw))
>> +						return not_found(pvmw);
>> +					return true;
>> +				}
>> +				/* device-private pmd was split under us: handle on pte level */
>> +				spin_unlock(pvmw->ptl);
>> +				pvmw->ptl = NULL;
>> +			} else {
>> +				if ((pvmw->flags & PVMW_SYNC) &&
>> +				    thp_vma_suitable_order(vma, pvmw->address,
>> +							   PMD_ORDER) &&
>> +				    (pvmw->nr_pages >= HPAGE_PMD_NR))
>> +					sync_with_folio_pmd_zap(mm, pvmw->pmd);
>> +
>> +				step_forward(pvmw, PMD_SIZE);
>> +				continue;
>> +			}
>>  		}
>>  		if (!map_pte(pvmw, &pmde, &ptl)) {
>>  			if (!pvmw->pte)
>> --
>> 2.34.1
>>
>
>Thanks, Lorenzo
>
>----8<----
>>From e6a3c1c782714ed831c4d46a14bb99226423bf59 Mon Sep 17 00:00:00 2001
>From: Wei Yang <richard.weiyang@xxxxxxxxx>
>Date: Mon, 22 Jun 2026 13:06:51 +0000
>Subject: [PATCH] refactored
>
>Signed-off-by: Lorenzo Stoakes <ljs@xxxxxxxxxx>
>---
> mm/page_vma_mapped.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
>diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>index 2ccbabfb2cc1..17dff8aab9f9 100644
>--- a/mm/page_vma_mapped.c
>+++ b/mm/page_vma_mapped.c
>@@ -269,14 +269,24 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> 			/* 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);
>+		} else if (pmd_is_device_private_entry(pmde)) {
>+			softleaf_t entry;
>+
>+			pvmw->ptl = pmd_lock(mm, pvmw->pmd);
>+			pmde = *pvmw->pmd;
>+			entry = softleaf_from_pmd(pmde);
>
>-			if (softleaf_is_device_private(entry)) {
>-				pvmw->ptl = pmd_lock(mm, pvmw->pmd);
>+			if (likely(softleaf_is_device_private(entry))) {
>+				if (pvmw->flags & PVMW_MIGRATION)
>+					return not_found(pvmw);
>+				if (!check_pmd(softleaf_to_pfn(entry), pvmw))
>+					return not_found(pvmw);
> 				return true;
> 			}
>-
>+			/* device-private pmd was split under us: handle on pte level */
>+			spin_unlock(pvmw->ptl);
>+			pvmw->ptl = NULL;
>+		} else if (!pmd_present(pmde)) {
> 			if ((pvmw->flags & PVMW_SYNC) &&
> 			    thp_vma_suitable_order(vma, pvmw->address,
> 						   PMD_ORDER) &&
>--
>2.54.0

If we prefer this way, I will check and take it.

-- 
Wei Yang
Help you, Help me