Re: [PATCH 6/9] mm/compaction: trust zone->contiguous only in the span

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/compaction: pageblock_pfn_to_page() ignores end_pfn in span check
- [High] mm/compaction: racy zone_spans_pfn() check in pageblock_pfn_to_page()
--

commit fc93d5609d6ae896a622cb4ad47040ba39704072
Author: Josef Bacik <josef@toxicpanda.com>

mm/compaction: trust zone->contiguous only in the span

This commit restricts the fast path in pageblock_pfn_to_page() so that
zone->contiguous is only trusted when start_pfn falls within the zone span,
and pairs smp_store_release() in set_zone_contiguous() with
smp_load_acquire() in pageblock_pfn_to_page().

> diff --git a/mm/page_alloc.h b/mm/page_alloc.h
> index 2b1017a91928c..e24d1203c5148 100644
> --- a/mm/page_alloc.h
> +++ b/mm/page_alloc.h
> @@ -216,7 +216,12 @@ 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 (zone->contiguous)
> +	/*
> +	 * A scanner may still be at a pfn that has left the zone: the zone is
> +	 * shrunk before set_zone_contiguous() says that it has no holes.
> +	 */
> +	if (smp_load_acquire(&zone->contiguous) &&
> +	    zone_spans_pfn(zone, start_pfn))
>  		return pfn_to_page(start_pfn);

[Severity: High]
Can pageblock_pfn_to_page() still return a non-NULL page for a range that has
left the zone because it only checks start_pfn and ignores end_pfn?

When the lowest memory block at the start of a multi-block zone is offlined
and removed while compact_zone() is running with cc->migrate_pfn in that
removed lower block, remove_pfn_range_from_zone() advances
zone->zone_start_pfn via shrink_zone_span() and sets zone->contiguous = true
via set_zone_contiguous() when the remaining zone has no holes.

On the next call to compact_zone() -> isolate_migratepages() in
mm/compaction.c, block_start_pfn is clamped up to cc->zone->zone_start_pfn
while low_pfn and block_end_pfn remain in the removed range below
cc->zone->zone_start_pfn:

mm/compaction.c:isolate_migratepages() {
	low_pfn = fast_find_migrateblock(cc);
	block_start_pfn = pageblock_start_pfn(low_pfn);
	if (block_start_pfn < cc->zone->zone_start_pfn)
		block_start_pfn = cc->zone->zone_start_pfn;
	...
	block_end_pfn = pageblock_end_pfn(low_pfn);
	...
		page = pageblock_pfn_to_page(block_start_pfn,
						block_end_pfn, cc->zone);
	...
		if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,
						isolate_mode))
			return ISOLATE_ABORT;
}

Because block_start_pfn was clamped to cc->zone->zone_start_pfn,
zone_spans_pfn(zone, start_pfn) evaluates to true even though
start_pfn >= end_pfn and end_pfn is in the removed range.
pageblock_pfn_to_page() then returns pfn_to_page(start_pfn), and
isolate_migratepages() passes low_pfn and block_end_pfn to
isolate_migratepages_block(), which dereferences the unmapped or poisoned
memmap for low_pfn.

The same start_pfn clamping occurs in isolate_migratepages_range(),
isolate_freepages_range(), and fast_isolate_around():

mm/compaction.c:fast_isolate_around() {
	start_pfn = max(pageblock_start_pfn(pfn), cc->zone->zone_start_pfn);
	end_pfn = min(pageblock_end_pfn(pfn), zone_end_pfn(cc->zone));

	page = pageblock_pfn_to_page(start_pfn, end_pfn, cc->zone);
}

When pfn < cc->zone->zone_start_pfn, start_pfn > end_pfn, so
pageblock_pfn_to_page() returns pfn_to_page(cc->zone->zone_start_pfn) and
fast_isolate_around() -> isolate_freepages_block() falsely sets the pageblock
skip hint at the new start of the zone. Additionally, if zone_end_pfn(zone) is
not pageblock-aligned or shrinks from the top while remaining contiguous,
start_pfn < zone_end_pfn(zone) < end_pfn allows isolate_migratepages() to scan
past zone_end_pfn(zone).

Note that __pageblock_pfn_to_page() in mm/page_alloc.c also does not check
that start_pfn < end_pfn or that end_pfn - 1 is online when start_pfn has been
clamped into the online zone:

mm/page_alloc.c:__pageblock_pfn_to_page() {
	end_pfn--;

	if (!pfn_valid(end_pfn))
		return NULL;

	start_page = pfn_to_online_page(start_pfn);
}

Should pageblock_pfn_to_page() and __pageblock_pfn_to_page() verify that
start_pfn < end_pfn and that both start_pfn and end_pfn - 1 are spanned and
online?

[Severity: High]
Is reading zone->contiguous before zone_spans_pfn(zone, start_pfn) without
zone->span_seqlock safe against concurrent zone span changes?

Reading zone->contiguous first and the zone span second only pairs with the
false to true transition in set_zone_contiguous(). On the true to false
transition in move_pfn_range_to_zone() and remove_pfn_range_from_zone() in
mm/memory_hotplug.c, clear_zone_contiguous() clears zone->contiguous with a
plain store before the zone span is modified:

mm/mm_init.h:clear_zone_contiguous() {
	zone->contiguous = false;
}

mm/memory_hotplug.c:move_pfn_range_to_zone() {
	clear_zone_contiguous(zone);

	if (zone_is_empty(zone))
		init_currently_empty_zone(zone, start_pfn, nr_pages);
	resize_zone_range(zone, start_pfn, nr_pages);
	resize_pgdat_range(pgdat, start_pfn, nr_pages);
	...
	memmap_init_range(nr_pages, nid, zone_idx(zone), start_pfn, 0,
			 MEMINIT_HOTPLUG, altmap, migratetype,
			 isolate_pageblock);
}

Because the writer clears zone->contiguous first and updates the span second,
a reader in pageblock_pfn_to_page() executing in the same order can load
zone->contiguous == true just before clear_zone_contiguous(zone) and then
observe the expanded span from resize_zone_range() in
zone_spans_pfn(zone, start_pfn). When a contiguous zone is expanded across a
PFN hole or before memmap_init_range() initializes the new range and its
sections are marked online, a compaction scanner with a PFN in that hole or
new range can bypass __pageblock_pfn_to_page() and access an unmapped hole or
uninitialized memmap.

Furthermore, zone_spans_pfn() in pageblock_pfn_to_page() reads
zone->zone_start_pfn and zone->spanned_pages without zone->span_seqlock
(which remains unguarded even after later commits in the series resize zones
under span_seqlock and check pageblock pfns under span_seqlock in
__pageblock_pfn_to_page()):

mm/memory_hotplug.c:shrink_zone_span() {
		if (pfn) {
			zone->spanned_pages = zone_end_pfn(zone) - pfn;
			zone->zone_start_pfn = pfn;
		} else {
			zone->zone_start_pfn = 0;
			zone->spanned_pages = 0;
		}
}

When shrink_zone_span() raises zone->zone_start_pfn and reduces
zone->spanned_pages, a reader in pageblock_pfn_to_page() can observe the new
zone->zone_start_pfn together with the old zone->spanned_pages, causing
zone_end_pfn(zone) to transiently extend past the actual upper end of the
zone into a previously removed PFN range.

Should pageblock_pfn_to_page() read the zone span and zone->contiguous under
zone->span_seqlock, and ensure clear_zone_contiguous() is properly ordered
against span updates?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-pgalloc-ai-hotfixes-v1-0-9c7d1aca801e@kernel.org?part=6