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

["David Hildenbrand (Arm)" <david@kernel.org>]

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.

I can then follow up with a proper cleanup.

> 
> I think in general Andrew prefers separate fixes patches so I'd just make the
> _minimal_ change that fixes this for the backport, and the cleanup stuff as a
> separate series.
> 

The issue is that the existing handling is just crap, and to fix it, we're
adding more crap. But yeah, let's add more crap first before we clean it up
properly.


-- 
Cheers,

David