Re: [PATCH] drm/pagemap: Fix folio allocation fallback and use-after-put

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The order-0 allocation fallback loop causes a heap buffer overflow and memory leaks when migrating a partial THP or when some subpages lack the MIGRATE_PFN_MIGRATE flag.
--

--- Patch [1]: [PATCH] drm/pagemap: Fix folio allocation fallback and use-after-put ---
commit 0e0b48c51914bc3498ff387aa623528489adafed
Author: Matthew Brost <[email protected]>

drm/pagemap: Fix folio allocation fallback and use-after-put

This commit fixes a missing __GFP_NOWARN in higher-order folio allocations
and implements a fallback to order-0 allocations when THP allocation fails.
It also fixes a use-after-free issue where folio_order() was called after
dropping the page reference.

Cc: [email protected]

> diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> index 15c78eca180b7..407c7546e168a 100644
> --- a/drivers/gpu/drm/drm_pagemap.c
> +++ b/drivers/gpu/drm/drm_pagemap.c
[ ... ]
> @@ -886,11 +886,34 @@ static int drm_pagemap_migrate_populate_ram_pfn(struct vm_area_struct *vas,
>  
>  		order = folio_order(page_folio(src_page));
>  
> -		/* TODO: Support fallback to single pages if THP allocation fails */
>  		if (vas)
> -			folio = vma_alloc_folio(GFP_HIGHUSER, order, vas, addr);
> +			folio = vma_alloc_folio(GFP_HIGHUSER | __GFP_NOWARN, order, vas, addr);
>  		else
> -			folio = folio_alloc(GFP_HIGHUSER, order);
> +			folio = folio_alloc(GFP_HIGHUSER | __GFP_NOWARN, order);
> +
> +		if (!folio && order) {
> +			/*
> +			 * Higher-order allocation failed, fall back to
> +			 * order-0 allocations for the entire range covered
> +			 * by the original higher-order allocation, without
> +			 * setting MIGRATE_PFN_COMPOUND, until we move past
> +			 * that range.
> +			 */
> +			unsigned long nr = NR_PAGES(order);
> +			unsigned long j;
> +
> +			for (j = 0; j < nr; j++, i++, addr += PAGE_SIZE) {
> +				folio = vas ?
> +					vma_alloc_folio(GFP_HIGHUSER, 0, vas, addr) :
> +					folio_alloc(GFP_HIGHUSER, 0);
> +				if (!folio)
> +					goto free_pages;
> +
> +				page = folio_page(folio, 0);
> +				mpfn[i] = migrate_pfn(page_to_pfn(page));

[Severity: Critical]
Can this fallback loop write past the end of the mpfn array?

If the migration request only covers a partial THP, or if we are near the
end of the npages limit, nr could be larger than the remaining entries 
(npages - i). Since this loop unconditionally increments i and writes to 
mpfn[i] nr times, it appears it could cause a heap buffer overflow.

Additionally, this loop doesn't check if the subpages have the
MIGRATE_PFN_MIGRATE flag set in src_mpfn. Can this cause memory leaks if we
allocate and assign order-0 folios for subpages that weren't actually
requested for migration and thus might not be cleaned up later?

> +			}
> +			continue;
> +		}
>  
>  		if (!folio)
>  			goto free_pages;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1