Re: [PATCH v3] mm/migrate_device: fix pgtable leak in migrate_vma_insert_huge_pmd_page

Re: [PATCH v3] mm/migrate_device: fix pgtable leak in migrate_vma_insert_huge_pmd_page

[Huang, Ying]

"David Hildenbrand (Arm)" <david@kernel.org> writes:

> On 5/1/26 14:44, Andrew Morton wrote:
>> On Fri,  1 May 2026 17:21:16 +0530 Sunny Patel <nueralspacetech@gmail.com> wrote:
>> 
>>> When migrate_vma_insert_huge_pmd_page() jumps to unlock_abort due
>>> to a PMD check failure, the pgtable allocated earlier via
>>> pte_alloc_one() is never freed, causing a memory leak.
>>>
>>> Added free_abort label to release the pgtable in error path.
>>>
>>> ...
>>>
>>> --- a/mm/migrate_device.c
>>> +++ b/mm/migrate_device.c
>>> @@ -840,7 +840,7 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>>>  	} else {
>>>  		if (folio_is_zone_device(folio) &&
>>>  		    !folio_is_device_coherent(folio)) {
>>> -			goto abort;
>>> +			goto free_abort;
>>>  		}
>>>  		entry = folio_mk_pmd(folio, vma->vm_page_prot);
>>>  		if (vma->vm_flags & VM_WRITE)
>>> @@ -893,6 +893,8 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>>>  
>>>  unlock_abort:
>>>  	spin_unlock(ptl);
>>> +free_abort:
>>> +	pte_free(vma->vm_mm, pgtable);
>>>  abort:
>>>  	for (i = 0; i < HPAGE_PMD_NR; i++)
>>>  		src[i] &= ~MIGRATE_PFN_MIGRATE;
>> 
>> Yikes, we leak that page on several error paths.
>> 
>> Thanks, I'll retain David's ack from the v2 patch.
>
> Yes. If we want to avoid more labels, we could do something like:
>
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index ab49d4dcdb60..babb56c4d47f 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -795,8 +795,8 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>         struct folio *folio = page_folio(page);
>         int ret;
>         vm_fault_t csa_ret;
> -       spinlock_t *ptl;
> -       pgtable_t pgtable;
> +       spinlock_t *ptl = NULL;
> +       pgtable_t pgtable = NULL;
>         pmd_t entry;
>         bool flush = false;
>         unsigned long i;
> @@ -818,14 +818,14 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>                 count_vm_event(THP_FAULT_FALLBACK);
>                 count_mthp_stat(HPAGE_PMD_ORDER, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
>                 ret = -ENOMEM;
> -               goto abort;
> +               goto error;
>         }
>  
>         __folio_mark_uptodate(folio);
>  
>         pgtable = pte_alloc_one(vma->vm_mm);
>         if (unlikely(!pgtable))
> -               goto abort;
> +               goto error;
>  
>         if (folio_is_device_private(folio)) {
>                 swp_entry_t swp_entry;
> @@ -840,7 +840,7 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>         } else {
>                 if (folio_is_zone_device(folio) &&
>                     !folio_is_device_coherent(folio)) {
> -                       goto abort;
> +                       goto error;
>                 }
>                 entry = folio_mk_pmd(folio, vma->vm_page_prot);
>                 if (vma->vm_flags & VM_WRITE)
> @@ -850,21 +850,21 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>         ptl = pmd_lock(vma->vm_mm, pmdp);
>         csa_ret = check_stable_address_space(vma->vm_mm);
>         if (csa_ret)
> -               goto unlock_abort;
> +               goto error;
>  
>         /*
>          * Check for userfaultfd but do not deliver the fault. Instead,
>          * just back off.
>          */
>         if (userfaultfd_missing(vma))
> -               goto unlock_abort;
> +               goto error;
>  
>         if (!pmd_none(*pmdp)) {
>                 if (!is_huge_zero_pmd(*pmdp))
> -                       goto unlock_abort;
> +                       goto error;
>                 flush = true;
>         } else if (!pmd_none(*pmdp))
> -               goto unlock_abort;
> +               goto error;
>  
>         add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
>         folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE);
> @@ -891,9 +891,11 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>  
>         return 0;
>  
> -unlock_abort:
> -       spin_unlock(ptl);
> -abort:
> +error:
> +       if (ptl)
> +               spin_unlock(ptl);
> +       if (pgtable)
> +               pte_free(vma->vm_mm, pgtable);
>         for (i = 0; i < HPAGE_PMD_NR; i++)
>                 src[i] &= ~MIGRATE_PFN_MIGRATE;
>         return 0;

Both look good to me, feel free to add my

Reviewed-by: Huang Ying <ying.huang@linux.alibaba.com>

in the future versions.

---
Best Regards,
Huang, Ying

Re: [PATCH v3] mm/migrate_device: fix pgtable leak in migrate_vma_insert_huge_pmd_page

[David Hildenbrand (Arm)]

>> -unlock_abort:
>> -       spin_unlock(ptl);
>> -abort:
>> +error:
>> +       if (ptl)
>> +               spin_unlock(ptl);
>> +       if (pgtable)
>> +               pte_free(vma->vm_mm, pgtable);
>>         for (i = 0; i < HPAGE_PMD_NR; i++)
>>                 src[i] &= ~MIGRATE_PFN_MIGRATE;
>>         return 0;
>>
>>
> 
> The changes look good!
> 
> Reviewed-by: Balbir Singh <balbirs@nvidia.com>

Do we prefer that or what we have in v3?

In case we prefer what I posted, it would be great if Sunny could send a v4!

-- 
Cheers,

David