Re: [PATCH v2 2/5] mm/page_alloc: validate zonelist slots against concurrent rebuilds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/page_alloc: stale preferred_zoneref after zonelist_zone_valid() skips slot
--

commit c301c4c2a3915ab844aff8ba7e717c949c45cabd
Author: Gregory Price (Meta) <gourry@gourry.net>

mm/page_alloc: validate zonelist slots against concurrent rebuilds

This commit adds zonelist_zone_valid() to recheck the loaded zone pointer
against highest_zoneidx and the nodemask in get_page_from_freelist() and
alloc_pages_bulk_noprof(), preventing in-flight zonelist walks from using an
ineligible zone after a concurrent hotplug rebuild shifts zonelist slots.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c7125f6693574..aa3cd44fdaa5b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3784,6 +3784,19 @@ static inline unsigned int alloc_flags_cma(gfp_t gfp_mask)
>  	return ALLOC_DEFAULT;
>  }
>  
> +/*
> + * The zonelist can be rebuilt in place while it is walked, so the iterator's
> + * filters may have passed a slot that now holds a different zone. Recheck the
> + * zone itself.
> + */
> +static inline bool zonelist_zone_valid(struct zone *zone,
> +				       const struct alloc_context *ac)
> +{
> +	if (zone_idx(zone) > ac->highest_zoneidx)
> +		return false;
> +	return !ac->nodemask || node_isset(zone_to_nid(zone), *ac->nodemask);
> +}
> +
>  /*
>   * get_page_from_freelist goes through the zonelist trying to allocate
>   * a page.
> @@ -3812,6 +3825,9 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>  		struct page *page;
>  		unsigned long mark;
>  
> +		if (unlikely(!zonelist_zone_valid(zone, ac)))
> +			continue;
> +
>  		if (cpusets_enabled() &&
>  			(alloc_flags & ALLOC_CPUSET) &&
>  			!__cpuset_zone_allowed(zone, gfp_mask))
> @@ -5236,6 +5252,9 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  	for_next_zone_zonelist_nodemask(zone, z, ac.highest_zoneidx, ac.nodemask) {
>  		unsigned long mark;
>  
> +		if (unlikely(!zonelist_zone_valid(zone, &ac)))
> +			continue;
> +
>  		if (cpusets_enabled() && (alloc_flags & ALLOC_CPUSET) &&
>  		    !__cpuset_zone_allowed(zone, gfp)) {
>  			continue;

[Severity: Medium]
When zonelist_zone_valid() rejects and skips the initial slot at
z == ac->preferred_zoneref after a concurrent online_pages() ->
build_all_zonelists() shifts an ineligible zone (such as ZONE_MOVABLE for a
GFP_KERNEL request, or a zone from a node outside ac->nodemask) into that
slot, does ac->preferred_zoneref remain pointing at the rejected slot for the
rest of the walk?

In both get_page_from_freelist() and alloc_pages_bulk_noprof(), the walk
starts at z = ac->preferred_zoneref (and in the follow-up commit
"mm/page_alloc: snapshot the preferred zone against shrink rebuilds",
preferred_zone = READ_ONCE(ac->preferred_zoneref->zone) is snapshotted before
the loop without validating or updating it when the initial slot is skipped).

In alloc_pages_bulk_noprof(), subsequent iterations still consult
ac.preferred_zoneref for the local-node check and watermark check, as well as
for statistics at the end of the function:

mm/page_alloc.c:alloc_pages_bulk_noprof() {
    ...
		if (nr_online_nodes > 1 && zone != zonelist_zone(ac.preferred_zoneref) &&
		    zone_to_nid(zone) != zonelist_node_idx(ac.preferred_zoneref)) {
			goto failed;
		}

		cond_accept_memory(zone, 0, alloc_flags);
retry_this_zone:
		mark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK) + nr_pages - nr_populated;
		if (zone_watermark_fast(zone, 0,  mark,
				zonelist_zone_idx(ac.preferred_zoneref),
				alloc_flags, gfp)) {
			break;
		}
    ...
	__count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
	zone_statistics(zonelist_zone(ac.preferred_zoneref), zone, nr_account);
}

If a zone from a disallowed node was shifted into ac.preferred_zoneref, the
node comparison against zonelist_node_idx(ac.preferred_zoneref) will trigger
on the first allowed zone in ac.nodemask and jump to failed, aborting the
bulk allocation into the single-page fallback. Additionally,
zone_watermark_fast() receives zonelist_zone_idx(ac.preferred_zoneref) as
highest_zoneidx, indexing zone->lowmem_reserve[] with the rejected zone's
index (for example, ZONE_MOVABLE for a GFP_KERNEL allocation).

Similarly, in get_page_from_freelist(), later iterations still reference
ac->preferred_zoneref:

mm/page_alloc.c:get_page_from_freelist() {
    ...
		if (no_fallback && !defrag_mode && nr_online_nodes > 1 &&
		    zone != zonelist_zone(ac->preferred_zoneref)) {
			int local_nid;

			local_nid = zonelist_node_idx(ac->preferred_zoneref);
			if (zone_to_nid(zone) != local_nid) {
				alloc_flags &= ~ALLOC_NOFRAGMENT;
				goto retry;
			}
		}
    ...
			if (!node_reclaim_enabled() ||
			    !zone_allows_reclaim(zonelist_zone(ac->preferred_zoneref), zone))
				continue;
    ...
try_this_zone:
		page = rmqueue(zonelist_zone(ac->preferred_zoneref), zone, order,
				gfp_mask, alloc_flags, ac->migratetype);
}

Could this cause get_page_from_freelist() to treat the first valid zone on
the allowed node as remote and prematurely strip ALLOC_NOFRAGMENT, as well as
pass the rejected zone to zone_allows_reclaim() and rmqueue() ->
zone_statistics() (and prior to the follow-up commit snapshotting
preferred_zone, risk a NULL pointer dereference if a concurrent offline
shrinks the zonelist so ac->preferred_zoneref->zone becomes NULL)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019172346.3344005-1-gourry@gourry.net?part=2