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

[Wei Yang <richard.weiyang@gmail.com>]

On Fri, Jun 19, 2026 at 12:48:26PM +0200, David Hildenbrand (Arm) wrote:
>On 6/19/26 12:44, Lorenzo Stoakes wrote:
>> -cc wrong email
>> 
>> On Tue, Jun 16, 2026 at 06:34:36AM +0000, Wei Yang wrote:
>>> For pmd_trans_huge() and pmd_is_migration_entry(), we does following
>>> before return the pmd entry:
>>>
>>>   * 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().
>>>
>>> This may return improper entry, e.g. if we are looking for a migration
>>> entry, device-private entry could still be returned, which leads to data
>>> corruption.
>> 
>> I don't thik this is quite clear?
>> 
>> How about:
>> 
>> 	If a softleaf entry is present, 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 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.
>>>
>>> While at it, it cleanups the pmd entry handling in page_vma_mapped_walk().
>>>
>>>   * Instead of handling trans huge/migration entry/device private entry
>>>     in a mixed manner, we put each case into its own if condition and
>>>     handle with the same pattern.
>>>   * Also we grab PTL and make sure pmd is not changed under us after
>>>     above check instead of do the check with PTL hold.
>>>   * restart the process if pmd is changed under us
>> 
>> You're doing quite a bit for a fix and you're putting it all in one place.
>> 
>> How about do the fix as 1 patch, and then cleanups as other ones? It helps with
>> review too :)
>> 
>> It's a general rule of thumb that if you do more than one of moving, refactoring
>> or changing code, to do them as separate patches so a reviewer/somebody
>> bisecting can clearly separate each.
>> 
>> Also PLEASE do not add new functionality (this lock recheck) in a fixes
>> patch. We'll end up backporting new logic that way.
>> 
>> Make the fixes bit _minimal_.
>
>To be fair, I asked for this
>
>https://lore.kernel.org/all/2d48ef0d-1110-4a9d-adcb-f701a1ce2cfa@kernel.org/
>
>But given that Wei mostly used my quick draft without properly checking the
>implications, yeah, let's fix it first separately.

Sorry, if I misunderstand your point.

>
>I can then follow up with a proper cleanup.
>

I would like to do a followup cleanup for this, may I have this chance?

-- 
Wei Yang
Help you, Help me