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

[Vlastimil Babka (SUSE)]

On 10/19/26 19:23, Gregory Price (Meta) wrote:
> Onlining the first block of a zone rebuilds every zonelist in place,
> shifting the slots after the new zone. An in-flight walk can use a
> zone it previously filtered. e.g.:
>   - a GFP_KERNEL request can land in ZONE_MOVABLE,
>   - a node not in the nodemask can have its zones selected.
> 
> With kprobes holding the walk across the online, I was able to get this
> zone/node misplacement to happen consistently across hotplug events.
> 
> The iterator approves a slot by its zone index and node, and the
> allocator then loads the zone pointer from the slot again:
> 
>   CPU 0                             CPU 1
>   first_zones_zonelist(nodemask {1})
>     slot 0 (NORMAL1) approved
>                                     online_pages()
>                                       build_all_zonelists()
>                                         [NORMAL1, NORMAL2, ...]
>                                         -> [MOVABLE1, NORMAL1, ...]
>   get_page_from_freelist()
>     zone = zonelist_zone(slot 0)    /* MOVABLE1 */

Since the zonelist_zone_valid() checks you're adding happen before the
"zone = zonelist_zone(slot 0)" part, I don't understand how they can be
helping here. Aren't we just reapproving slot 0 when CPU 1 can mess it up
between that reapproval and load? Am I missing something?

Furthermore, since we have the zonelist_update_seq anyway, wouldn't it be
better to use it to avoid these kinds of races rather than the more tricky
way of relying on what exact updates in what order can derail the allocator
side and trying to protect against those specifically?

> Every later step of the walk filters and loads the same way, so a slot
> approved for one node can also hold another node's zone by that time.
> 
> Recheck the loaded zone against highest_zoneidx and the nodemask in both
> allocator loops, and skip it on a mismatch. Hot-add only inserts zones,
> so the zone that was in a skipped slot is still ahead in the walk, and a
> walk that runs off the end meanwhile is retried by the zonelist seq-check.
> 
> The recheck adds about a dozen instructions per zone visited. The two
> fields it reads share a cacheline with zone->per_cpu_pageset, which the
> pcp allocation path loads anyway.
> 
> Fixes: c33d6c06f60f7 ("mm, page_alloc: avoid looking up the first zone in a zonelist twice")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
> ---
>  mm/page_alloc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> 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;