Re: [PATCH 5.15.y] mm/huge_memory: fix dereferencing invalid pmd migration entry

[Gavin Guo <gavinguo@igalia.com>]

On 6/19/25 11:30, Hugh Dickins wrote:
> On Mon, 16 Jun 2025, Gavin Guo wrote:
> 
>> [ Upstream commit be6e843fc51a584672dfd9c4a6a24c8cb81d5fb7 ]
>>
>> When migrating a THP, concurrent access to the PMD migration entry during
>> a deferred split scan can lead to an invalid address access, as
>> illustrated below.  To prevent this invalid access, it is necessary to
>> check the PMD migration entry and return early.  In this context, there is
>> no need to use pmd_to_swp_entry and pfn_swap_entry_to_page to verify the
>> equality of the target folio.  Since the PMD migration entry is locked, it
>> cannot be served as the target.
>>
>> Mailing list discussion and explanation from Hugh Dickins: "An anon_vma
>> lookup points to a location which may contain the folio of interest, but
>> might instead contain another folio: and weeding out those other folios is
>> precisely what the "folio != pmd_folio((*pmd)" check (and the "risk of
>> replacing the wrong folio" comment a few lines above it) is for."
>>
>> BUG: unable to handle page fault for address: ffffea60001db008
>> CPU: 0 UID: 0 PID: 2199114 Comm: tee Not tainted 6.14.0+ #4 NONE
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
>> RIP: 0010:split_huge_pmd_locked+0x3b5/0x2b60
>> Call Trace:
>> <TASK>
>> try_to_migrate_one+0x28c/0x3730
>> rmap_walk_anon+0x4f6/0x770
>> unmap_folio+0x196/0x1f0
>> split_huge_page_to_list_to_order+0x9f6/0x1560
>> deferred_split_scan+0xac5/0x12a0
>> shrinker_debugfs_scan_write+0x376/0x470
>> full_proxy_write+0x15c/0x220
>> vfs_write+0x2fc/0xcb0
>> ksys_write+0x146/0x250
>> do_syscall_64+0x6a/0x120
>> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> The bug is found by syzkaller on an internal kernel, then confirmed on
>> upstream.
>>
>> Link: https://lkml.kernel.org/r/20250421113536.3682201-1-gavinguo@igalia.com
>> Link: https://lore.kernel.org/all/20250414072737.1698513-1-gavinguo@igalia.com/
>> Link: https://lore.kernel.org/all/20250418085802.2973519-1-gavinguo@igalia.com/
>> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path")
>> Signed-off-by: Gavin Guo <gavinguo@igalia.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Acked-by: Hugh Dickins <hughd@google.com>
>> Acked-by: Zi Yan <ziy@nvidia.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>> Cc: Florent Revest <revest@google.com>
>> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
>> Cc: Miaohe Lin <linmiaohe@huawei.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> [gavin: backport the migration checking logic to __split_huge_pmd]
>> Signed-off-by: Gavin Guo <gavinguo@igalia.com>
>> ---
>>   mm/huge_memory.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 9139da4baa39..bcefc17954d6 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2161,7 +2161,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>   	VM_BUG_ON(freeze && !page);
>>   	if (page) {
>>   		VM_WARN_ON_ONCE(!PageLocked(page));
>> -		if (page != pmd_page(*pmd))
>> +		if (is_pmd_migration_entry(*pmd) || page != pmd_page(*pmd))
>>   			goto out;
>>   	}
>>   
>> @@ -2196,7 +2196,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>   		}
>>   		if (PageMlocked(page))
>>   			clear_page_mlock(page);
>> -	} else if (!(pmd_devmap(*pmd) || is_pmd_migration_entry(*pmd)))
>> +	} else if (!pmd_devmap(*pmd))
>>   		goto out;
> 
> I'm sorry, Gavin, but this 5.15 and the 5.10 and 5.4 backports look wrong
> to me, because here you drop the is_pmd_migration_entry(*pmd) condition,
> but if !page then that has not been checked earlier (this check here is
> specifically allowing a pmd migration entry to proceed to the split).
> 
> Hugh

Hi Hugh,

Thank you again for the review.

Regarding the 5.4/5.10/5.15. How do you think about the following changes?

@@ -2327,6 +2327,8 @@ void __split_huge_pmd(struct vm_area_struct *vma, 
pmd_t *pmd,
         mmu_notifier_invalidate_range_start(&range);
         ptl = pmd_lock(vma->vm_mm, pmd);

+       if (is_pmd_migration_entry(*pmd))
+               goto out;
         /*
          * If caller asks to setup a migration entries, we need a page 
to check
          * pmd against. Otherwise we can end up replacing wrong page.
@@ -2369,7 +2371,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, 
pmd_t *pmd,
                 }
                 if (PageMlocked(page))
                         clear_page_mlock(page);
-       } else if (!(pmd_devmap(*pmd) || is_pmd_migration_entry(*pmd)))
+       } else if (!pmd_devmap(*pmd) )
                 goto out;
         __split_huge_pmd_locked(vma, pmd, range.start, freeze);
  out:

There is still an access, page = pmd_page(*pmd), inside the if(!page). 
I'm not sure if pmd could be a migration entry when the page is NULL. To 
avoid this as well, maybe just goto out directly in the beginning?

> 
>>   	__split_huge_pmd_locked(vma, pmd, range.start, freeze);
>>   out:
>>
>> base-commit: 1c700860e8bc079c5c71d73c55e51865d273943c
>> -- 
>> 2.43.0