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

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

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;


-- 
Cheers,

David