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

[Zi Yan]

On 2 Jun 2026, at 14:56, David Hildenbrand (Arm) wrote:

> On 6/2/26 17:02, Zi Yan wrote:
>> On 2 Jun 2026, at 9: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)) {
>>>  		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))
>>>  				return true;
>>> -
>>> -		} else if (!folio_test_lru(folio)) {
>>> +		} else if (!PageLRU(page)) {
>>>  			return true;
>>>  		}
>>>
>>> -		*step = folio_nr_pages(folio) - folio_page_idx(folio, page);
>>> +		nr_pages = 1UL << order;
>>> +		pfn = page_to_pfn(page);
>>> +		*step = (pfn | (nr_pages - 1)) + 1 - pfn;
>>>  		return false;
>>>  	}
>>
>> LGTM. Thanks.
>>
>> Just a comment, order can be dropped and use
>> nr_pages = compound_nr(&folio->page) instead:
>> 1. order > MAX_FOLIO_ORDER -> nr_pages > MAX_FOLIO_NR_PAGES
>> 2. PAGE_SIZE << order -> PAGE_SIZE * nr_pages.
>> But it is not worth a new version.
>
> If we use compound_nr, we have to check for power-of-2, like
> scan_movable_pages(),  as folio_large_nr_pages() might use the per-computed
> value (which might read garbage).

The power-of-2 requirement comes from “pfn | (nr_pages - 1)”, right?
Since otherwise the calculation is going to be bogus.

In terms of reading garbage from either compound_order() or compound_nr(),
the code might skip undesired pages. Neither is better.

For the sake of discussion, to use compound_nr(), we probably
can use *step = folio_pfn(folio) + nr_pages - page_to_pfn(page) instead.


Best Regards,
Yan, Zi