Re: [PATCH] mm/migrate_device: avoid overflowing migrate_vma collection arrays

[Zi Yan]

On Wed Jul 8, 2026 at 10:43 AM EDT, Zi Yan wrote:
> On Wed Jul 8, 2026 at 10:35 AM EDT, Zi Yan wrote:
>> On Wed Jul 8, 2026 at 4:43 AM EDT, David Hildenbrand (Arm) wrote:
>>> On 7/8/26 03:50, Zi Yan wrote:
>>>> migrate_vma_collect_pmd() can drop pte lock to split a large folio and
>>>> restart. But the code does not handle restart properly when pmd becomes
>>>> huge or cleared. It can overflow migrate->dst and migrate->src arrays
>>>> during the hole or skip collection. Fix it by:
>>>> 1. avoiding migrate_vma_collect_huge_pmd() if some collection is done,
>>>> 2. skipping the rest of the range if pmd no longer points to a pte page
>>>>    table.
>>>> 
>>>> Fixes: a30b48bf1b244 ("mm/migrate_device: implement THP migration of zone device pages")
>>>> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
>>>> Closes: https://sashiko.dev/#/patchset/20260706111958.3649651-1-wangkefeng.wang@xxxxxxxxxx
>>>> Assisted-by: Claude:claude-opus-4-8
>>>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>>>> ---
>>>> The issue is spot by Sashiko during a patch[1] review as a pre-existing one.
>>>> This patch has the minimal change. An alternative is to reset
>>>> migrate->cpages and migrate->npages and restart the whole range from the
>>>> beginning, but that also requires a restoration of no-longer-present PTEs.
>>>> 
>>>> Link: https://lore.kernel.org/all/20260706111958.3649651-1-wangkefeng.wang@xxxxxxxxxx/ [1]
>>>> ---
>>>>  mm/migrate_device.c | 19 +++++++++++++++++--
>>>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>>>> index 2fffeb1f99694..6ceb47ec1da24 100644
>>>> --- a/mm/migrate_device.c
>>>> +++ b/mm/migrate_device.c
>>>> @@ -257,7 +257,12 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>>>>  	pte_t *ptep;
>>>>  
>>>>  again:
>>>> -	if (pmd_trans_huge(*pmdp) || !pmd_present(*pmdp)) {
>>>> +	/*
>>>> +	 * Only check pmd when addr is at start, namely no pte is collected.
>>>> +	 * It avoids collecting the same address range [start, addr) twice
>>>> +	 * and overflowing the collection arrays.
>>>> +	 */
>>>> +	if (addr == start && (pmd_trans_huge(*pmdp) || !pmd_present(*pmdp))) {
>>>>  		int ret = migrate_vma_collect_huge_pmd(pmdp, start, end, walk, fault_folio);
>>>>  
>>>>  		if (ret == -EAGAIN)
>>>> @@ -267,8 +272,18 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>>>>  	}
>>>>  
>>>>  	ptep = pte_offset_map_lock(mm, pmdp, start, &ptl);
>>>> -	if (!ptep)
>>>> +	if (!ptep) {
>>>> +		/*
>>>> +		 * Skip the rest if pmd becomes huge or cleared. Flush if any
>>>> +		 * pte is modified
>>>> +		 */
>>>> +		if (addr != start) {
>>>> +			if (unmapped)
>>>> +				flush_tlb_range(walk->vma, start, end);
>>>> +			return migrate_vma_collect_skip(addr, end, walk);
>>>> +		}
>>>>  		goto again;
>>>> +	}
>>>>  	lazy_mmu_mode_enable();
>>>>  	ptep += (addr - start) / PAGE_SIZE;
>>>
>>> It's hard to express which feelings reading migrate_vma_collect_pmd() gives me,
>>> haha :)
>>
>> I guess I have the same feelings.
>>>
>>>
>>> In case we split ... couldn't we just undo what we already did, before doing the
>>> "goto again" ?
>>
>> You mean we reset migrate->cpages and migrate->npages and restart from
>> the beginning? But it is not only that, since the code below also
>> changes PTEs into migration entries. We will need to revert them as
>> well.
>>
>> Hmm, migrate_vma_collect() documents itself as "update the src array and
>> "takes a reference on the page" without mentioning changing PTEs. I
>> wonder why changing PTEs is necessary, since later migrate_vma_unmap()
>> also changes page table entries to migration entries, although
>> migrate_vma_unmap() changes all entries to a folio, whereas
>> migrate_vma_collect() only changes PTEs from the specified VMA.
>
> OK, it is an optimization[1] when migrate_vma*() was introduced. If
> there is only one mapping, migrate_vma_collect() will set migration
> entry immediately without waiting until migrate_vma_unmap(). Fun.
>
> [1] Commit 8c3328f1f36a5 ("mm/migrate: migrate_vma() unmap page from vma while collecting pages")

Hi David,

Do you think this patch is good to get in? I would like to get ack from
you.

Thanks.

-- 
Best Regards,
Yan, Zi