Re: [PATCH 1/2] mm/compaction: check the range to pageblock_pfn_to_page() is within the zone first

[Wei Yang <richard.weiyang@gmail.com>]

On Wed, Oct 08, 2025 at 12:14:25PM +0200, David Hildenbrand wrote:
>On 02.10.25 05:31, Wei Yang wrote:
>> Function pageblock_pfn_to_page() is introduced by commit 7d49d8868336
>> ("mm, compaction: reduce zone checking frequency in the migration scanner"),
>> where there is no requirement on start_pfn/end_pfn except they are in
>> the same pageblock.
>> 
>> So at that time, pageblock_pfn_to_page() would be passed with pfn
>> without compared with zone boundary.
>> 
>> But after commit 7cf91a98e607 ("mm/compaction: speed up
>> pageblock_pfn_to_page() when zone is contiguous"), pageblock_pfn_to_page()
>> would think the range is valid and in the same zone if zone->contiguous,
>> even the range doesn't belong to this zone.
>> 
>> For example, in fast_isolate_freepages(), min_pfn is assigned to
>> pageblock_start_pfn() and passed to pageblock_pfn_to_page() without
>> checking with zone_start_pfn. And mostly, the end_pfn is not checked
>> with zone_end_pfn() before using.
>> 
>> To make this function robust, check the range is within the zone first.
>> 
>> Fixes: 7cf91a98e607 ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous")
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> ---
>>   mm/internal.h | 3 +++
>>   1 file changed, 3 insertions(+)
>> 
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 38607b2821d9..8e1a3819c9f1 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -724,6 +724,9 @@ extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>>   static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
>>   				unsigned long end_pfn, struct zone *zone)
>>   {
>> +	if (start_pfn < zone->zone_start_pfn || end_pfn > zone_end_pfn(zone))
>> +		return NULL;
>> +
>>   	if (zone->contiguous)
>>   		return pfn_to_page(start_pfn);
>
>fast_isolate_around() adjusts the range to the zone boundaries before calling
>fast_isolate_around().
>

Correct.

>isolate_migratepages_range(), isolate_freepages_range(), isolate_freepages()
>and isolate_migratepages similarly all seem to take the zone range into
>account as well.

Maybe I missed something.

isolate_freepages() takes care of the end_pfn, but I don't see it adjust
start_pfn. Or you mean low_pfn is already adjusted?

isolate_freepages_range() / isolate_migratepages_range() just adjust start_pfn.

isolate_migratepages() seems not adjust start_pfn nor end_pfn?

Maybe I misunderstand you, would you mind giving more detail?

>
>Only fast_isolate_freepages is a bit more tricky. It definitely takes the end
>into account. The start comes from cc->free_pfn but IIRC it's always above
>the zone_start_pfn.
>

You mean we rely on the caller to make sure the range is in within zone?

>So I wonder -- if we want to add such a check --  whether it should instead
>be a VM_WARN_ON_ONCE()?
>
>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me