Re: [PATCH v2] mm: page_isolation: avoid unsafe folio reads while scanning compound pages

[David Hildenbrand (Arm)]

On 6/2/26 15:07, Kaitao Cheng wrote:
> From: Kaitao Cheng <chengkaitao@xxxxxxxxxx>
> 
> page_is_unmovable() can inspect compound pages without holding a folio
> reference or any lock. The folio can therefore be freed, split or reused
> while the scanner is still looking at it.
> 
> The existing HugeTLB handling already avoids folio_hstate() for this
> reason, but it still derives the hstate from folio_size() and later
> derives the scan step from folio_nr_pages() and folio_page_idx().
> These helpers rely on the folio still being a valid folio head. If
> the folio changed concurrently, the scanner can read inconsistent folio
> metadata and compute a wrong step. In the worst case, folio_nr_pages()
> can return 1 for what used to be a tail page and the subtraction from
> folio_page_idx() can underflow.
> 
> There is a similar issue for non-Hugetlb compound pages: folio_test_lru()
> expects a valid folio. If the previously observed head page has been
> reused as a tail page of another compound page, the folio flag checks
> can trigger VM_BUG_ON_PGFLAGS().
> 
> Read the compound order once with compound_order(), reject obviously
> bogus orders, and derive the hstate and scan step from that order
> instead of querying folio size information again. Also use PageLRU(page),
> which is safe for the page being scanned, instead of folio_test_lru()
> on a potentially stale folio pointer.
> 
> Treat an unknown HugeTLB hstate as unmovable so the scanner does not try
> to skip over an unstable HugeTLB folio.
> 
> Fixes: a0a9f2180b90 ("mm: page_isolation: avoid calling folio_hstate() without hugetlb_lock")
> Signed-off-by: Kaitao Cheng <chengkaitao@xxxxxxxxxx>
> ---
> Changes in v2:
> - Avoid unsafe folio metadata reads in the unlocked scanner by deriving
>   the hstate and scan step from compound_order(). (David Hildenbrand,
>   Andrew Morton)
> - Treat invalid compound orders or unknown HugeTLB hstates as unmovable.
> - Use PageLRU(page) instead of folio_test_lru(folio) to avoid folio flag
>   checks on a stale folio pointer. ()
> - Update the commit log (David Hildenbrand)
> 
> Link to v1:
> https://lore.kernel.org/all/20260519121646.40833-1-kaitao.cheng@xxxxxxxxx/
> 
> ---
>  mm/page_isolation.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 7a9d631945a3..32ce8a7d9df3 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -41,8 +41,14 @@ bool page_is_unmovable(struct zone *zone, struct page *page,
>  	 * We need not scan over tail pages because we don't
>  	 * handle each tail page individually in migration.
>  	 */
> -	if (PageHuge(page) || PageCompound(page)) {
> +	if (PageCompound(page)) {

Nit: you're not spelling that change out in the patch description, but it's alright.

>  		struct folio *folio = page_folio(page);
> +		unsigned long nr_pages, pfn;
> +		unsigned int order;
> +
> +		order = compound_order(&folio->page);
> +		if (order > MAX_FOLIO_ORDER)
> +			return true;
>  
>  		if (folio_test_hugetlb(folio)) {
>  			struct hstate *h;
> @@ -54,15 +60,16 @@ bool page_is_unmovable(struct zone *zone, struct page *page,
>  			 * The huge page may be freed so can not
>  			 * use folio_hstate() directly.
>  			 */
> -			h = size_to_hstate(folio_size(folio));
> -			if (h && !hugepage_migration_supported(h))
> +			h = size_to_hstate(PAGE_SIZE << order);
> +			if (!h || !hugepage_migration_supported(h))

This change might have been better in a separate patch, but it should also be
alright (and never really trigger, ever ...).

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David