Re: [PATCH mm-hotfixes] mm/huge_memory: use correct flags for device private PMD entry

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

On 6/1/26 17:50, Dev Jain wrote:
> 
> 
> On 01/06/26 2:00 pm, Lorenzo Stoakes wrote:
>> Commit 65edfda6f3f2 ("mm/rmap: extend rmap and migration support
>> device-private entries") updated set_pmd_migration_entry() to use
>> pmdp_huge_get_and_clear() in the softleaf case, but made no further
>> adjustments to the function itself.
>>
>> Therefore this function continues to incorrectly use pmd_write(),
>> pmd_soft_dirty() and pmd_uffd_wp() to determine whether the installed
>> migration entry should be marked writable, softdirty or uffd-wp
>> respectively.
>>
>> Whilst all are incorrect, the most problematic of these is pmd_write(), as
>> this can lead to corrupted rmap state.
>>
>> On x86-64 _PAGE_SWP_SOFT_DIRTY is aliased to _PAGE_RW. So calling
>> pmd_write() on a softleaf will return the softdirty state encoded in the
>> entry, assuming CONFIG_MEM_SOFT_DIRTY was enabled.
>>
>> This was observed when running the hmm.hmm_device_private.anon_write_child
>> selftest:
>>
>> 1. The test faults in a range then migrates it such that a device-private
>>    THP range is established.
>>
>> 2. The parent then migrates it to a device-private writable PMD entry whose
>>    folio is entirely AnonExclusive with entire_mapcount=1, softdirty set
>>    (accidentally correct write state).
>>
>> 3. The parent forks and the PMD entries are set to device-private read only
>>    entries, entire_mapcount=2, softdirty still set.
>>
>> 4. [BUG] The child writes to the range then migrates to RAM - intending to
>>    install non-writable migration entries - but replacing parent and child
>>    PMD mappings with WRITABLE entries due to misinterpreting the softdirty
>>    bit.
>>
>> 5. In remove_migration_pmd(), if !softleaf_is_migration_read(entry) we
>>    set the RMAP_EXCLUSIVE flag when calling folio_add_anon_rmap_pmd() for
>>    both parent and child, which are therefore AnonExclusive.
>>
>> 6. [SPLAT] Child sets migrated folio entire_mapcount=1, parent sets
>>    entire_mapcount=2 and we end up with an AnonExclusive folio with
>>    entire_mapcount=2! Assert fires in __folio_add_anon_rmap():
>>
>> 		VM_WARN_ON_FOLIO(folio_test_large(folio) &&
>> 				 folio_entire_mapcount(folio) > 1 &&
>> 				 PageAnonExclusive(cur_page), folio)
>>
>> This patch fixes the issue by correctly referencing the softleaf entry
>> fields for writable, softdirty and uffd-wp in set_pmd_migration_entry().
>>
>> It also only updates A/D flags if the entry is present as these are
>> otherwise not meaningful for a softleaf entry.
>>
>> This patch also flips the if (!present) { ... } else { ... } logic in
>> set_pmd_migration_entry() so it is easier to understand, and adds some
>> comments to make things clearer.
>>
>> I was able to bisect this to commit 775465fd26a3 ("lib/test_hmm: add zone
>> device private THP test infrastructure") which first exposes this bug as it
>> was the commit that permitted test_hmm to generate the test.
>>
>> However commit 65edfda6f3f2 ("mm/rmap: extend rmap and migration support
>> device-private entries") is the commit that actually enabled this
>> behaviour.
>>
>> Fixes: 65edfda6f3f2 ("mm/rmap: extend rmap and migration support device-private entries")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lorenzo Stoakes <ljs@kernel.org>
>> ---
> 
> Sashiko continues to find existing problems :) What do you think:
> 
> https://sashiko.dev/#/patchset/20260601083044.57132-1-ljs%40kernel.org

IIRC, Wei is already working on fixing what it reports here regarding
try_to_migrate_one(). So it's old news.

https://lore.kernel.org/r/20260508013728.21285-1-richard.weiyang@gmail.com

-- 
Cheers,

David