Re: [PATCH v2 3/5] mm/page_alloc: snapshot the preferred zone against shrink rebuilds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/page_alloc: unvalidated preferred_zone snapshot on zonelist rebuild
--

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

mm/page_alloc: snapshot the preferred zone against shrink rebuilds

This commit snapshots ac->preferred_zoneref->zone once in
get_page_from_freelist() and alloc_pages_bulk_noprof() to prevent a NULL
pointer dereference if a concurrent zonelist rebuild overwrites the preferred
slot with a NULL terminator mid-walk.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index aa3cd44fdaa5b..75a440ebcead6 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3807,12 +3807,21 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>  {
>  	struct zoneref *z;
>  	struct zone *zone;
> +	struct zone *preferred_zone;
>  	struct pglist_data *last_pgdat = NULL;
>  	bool last_pgdat_dirty_ok = false;
>  	bool no_fallback;
>  	bool skip_kswapd_nodes = nr_online_nodes > 1;
>  	bool skipped_kswapd_nodes = false;
>  
> +	/*
> +	 * A concurrent zonelist rebuild can NULL or replace this slot. Zones
> +	 * are never freed, so reading it once keeps a usable zone.
> +	 */
> +	preferred_zone = READ_ONCE(ac->preferred_zoneref->zone);
> +	if (!preferred_zone)
> +		return NULL;
> +
>  retry:
>  	/*
>  	 * Scan zonelist, looking for a zone with enough free.
> @@ -3862,7 +3871,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>  		}
>  
>  		if (no_fallback && !defrag_mode && nr_online_nodes > 1 &&
> -		    zone != zonelist_zone(ac->preferred_zoneref)) {
> +		    zone != preferred_zone) {
>  			int local_nid;
>  
>  			/*
> @@ -3870,7 +3879,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>  			 * fragmenting fallbacks. Locality is more important
>  			 * than fragmentation avoidance.
>  			 */
> -			local_nid = zonelist_node_idx(ac->preferred_zoneref);
> +			local_nid = zone_to_nid(preferred_zone);
>  			if (zone_to_nid(zone) != local_nid) {
>  				alloc_flags &= ~ALLOC_NOFRAGMENT;
>  				goto retry;
> @@ -3932,7 +3941,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>  				goto try_this_zone;
>  
>  			if (!node_reclaim_enabled() ||
> -			    !zone_allows_reclaim(zonelist_zone(ac->preferred_zoneref), zone))
> +			    !zone_allows_reclaim(preferred_zone, zone))
>  				continue;
>  
>  			if (!node_reclaim(zone->zone_pgdat, gfp_mask, order))
> @@ -3945,7 +3954,7 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>  		}
>  
>  try_this_zone:
> -		page = rmqueue(zonelist_zone(ac->preferred_zoneref), zone, order,
> +		page = rmqueue(preferred_zone, zone, order,
>  				gfp_mask, alloc_flags, ac->migratetype);
>  		if (page) {
>  			prep_new_page(page, order, gfp_mask, alloc_flags);
> @@ -5199,6 +5208,7 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  {
>  	struct page *page;
>  	struct zone *zone;
> +	struct zone *preferred_zone;
>  	struct zoneref *z;
>  	struct per_cpu_pages *pcp;
>  	struct list_head *pcp_list;
> @@ -5247,6 +5257,11 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  	if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, &ac, &gfp, &alloc_flags))
>  		goto out;
>  
> +	/* Read once; see get_page_from_freelist(). */
> +	preferred_zone = READ_ONCE(ac.preferred_zoneref->zone);
> +	if (!preferred_zone)
> +		goto failed;
> +
>  	/* Find an allowed local zone that meets the low watermark. */
>  	z = ac.preferred_zoneref;
>  	for_next_zone_zonelist_nodemask(zone, z, ac.highest_zoneidx, ac.nodemask) {
> @@ -5260,8 +5275,8 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  			continue;
>  		}
>  
> -		if (nr_online_nodes > 1 && zone != zonelist_zone(ac.preferred_zoneref) &&
> -		    zone_to_nid(zone) != zonelist_node_idx(ac.preferred_zoneref)) {
> +		if (nr_online_nodes > 1 && zone != preferred_zone &&
> +		    zone_to_nid(zone) != zone_to_nid(preferred_zone)) {
>  			goto failed;
>  		}
>  
> @@ -5269,7 +5284,7 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  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),
> +				zone_idx(preferred_zone),
>  				alloc_flags, gfp)) {
>  			break;
>  		}

[Severity: Medium]
What happens if build_all_zonelists() replaces ac->preferred_zoneref->zone
with an ineligible zone before preferred_zone is read?

After prepare_alloc_pages() or __alloc_pages_slowpath() initializes
ac->preferred_zoneref via first_zones_zonelist(), a concurrent zonelist
rebuild can overwrite that slot with a zone whose index exceeds
ac->highest_zoneidx (such as ZONE_MOVABLE for a GFP_KERNEL allocation) or a
zone on a node excluded by ac->nodemask.

Both get_page_from_freelist() and alloc_pages_bulk_noprof() only check
preferred_zone for NULL without checking zonelist_zone_valid(). Inside the
zonelist walk, for_next_zone_zonelist_nodemask() reloads
ac->preferred_zoneref->zone into zone and skips it when
!zonelist_zone_valid(zone, ac):

	z = ac->preferred_zoneref;
	for_next_zone_zonelist_nodemask(zone, z, ac->highest_zoneidx,
					ac->nodemask) {
		struct page *page;
		unsigned long mark;

		if (unlikely(!zonelist_zone_valid(zone, ac)))
			continue;

However, preferred_zone remains pointing to the rejected zone:

1. In alloc_pages_bulk_noprof(), zone_watermark_fast() receives
   zone_idx(preferred_zone) as highest_zoneidx. If preferred_zone was
   replaced by ZONE_MOVABLE while ac.highest_zoneidx is ZONE_NORMAL,
   __zone_watermark_ok() and zone_watermark_fast() will index
   zone->lowmem_reserve[ZONE_MOVABLE] instead of
   zone->lowmem_reserve[ZONE_NORMAL]. And if preferred_zone was replaced by a
   zone on a node outside ac.nodemask, the check
   zone_to_nid(zone) != zone_to_nid(preferred_zone) will abort the bulk
   allocation via goto failed as soon as it visits the first valid zone in
   ac.nodemask.

2. In get_page_from_freelist(), if preferred_zone was replaced by a zone on a
   disallowed node, visiting the first valid zone in ac->nodemask sees
   zone != preferred_zone and zone_to_nid(zone) != zone_to_nid(preferred_zone),
   prematurely clearing ALLOC_NOFRAGMENT and jumping to retry. It also passes
   the disallowed preferred_zone to zone_allows_reclaim() and to
   rmqueue() -> zone_statistics(), misaccounting local allocations on the
   first valid node as NUMA_MISS and NUMA_FOREIGN.

Should preferred_zone be validated with zonelist_zone_valid() (or set from
the first valid zone encountered in the loop) so that an invalidated slot is
not used as the preferred zone?

[ ... ]

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