* Re: [PATCH v3] mm/migrate_device: fix pgtable leak in migrate_vma_insert_huge_pmd_page
[not found] ` <24ab5ddc-11a9-40ed-90b2-1a6c68010928@kernel.org>
@ 2026-05-07 9:38 ` Huang, Ying
[not found] ` <6f92ad63-e601-425c-a61a-5ff929cba738@nvidia.com>
1 sibling, 0 replies; 2+ messages in thread
From: Huang, Ying @ 2026-05-07 9:38 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Sunny Patel, Zi Yan, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Alistair Popple,
linux-mm, linux-kernel, Balbir Singh
"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
^ permalink raw reply [flat|nested] 2+ messages in thread