Re: [PATCH v4 4/8] mm/mmzone: read each zoneref slot once when filtering by nodemask

[Vlastimil Babka (SUSE)]

On 10/28/26 22:19, Gregory Price (Meta) wrote:
> __next_zones_zonelist() tests a slot's zone for NULL, then reads the
> slot again to look up the zone's node. Offlining a zone's last block
> rebuilds the zonelists in place, so the second read can return the
> terminator:
> 
>   CPU 0                             CPU 1
>   __next_zones_zonelist(nodemask)
>     zonelist_zone(slot 1) = NORMAL
>                                     offline_pages()
>                                       build_all_zonelists()
>                                         [MOVABLE, NORMAL, NULL]
>                                         -> [NORMAL, NULL]
>     zref_in_nodemask(slot 1)
>       zone_to_nid(NULL)
> 
> Current GCC and clang merge the two reads, but nothing requires it.
> With mm/mmzone.o built with -fno-inline -fno-ipa-sra, holding a
> nodemask allocation between the reads while the node's last movable
> block goes offline oopses every time:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000060
>   RIP: 0010:zone_to_nid+0x0/0x10
>   Call Trace:
>    zref_in_nodemask+0x9/0x20
>    __next_zones_zonelist+0x23/0x70
>    prepare_alloc_pages.constprop.0+0x16c/0x190
>    __alloc_frozen_pages_noprof+0xaf/0x330
> 
> Read the slot once and test that zone. A NULL zone still ends the walk.
> 
> Fixes: 11cd8638c37f ("mm, page_alloc: remove stop_machine from build_all_zonelists")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>

Oh, zonelist_node_idx() reads nid from the zone, it's not part of zoneref as
I thought. So it's the same after this patch, just with READ_ONCE().

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

> ---
>  mm/mmzone.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index 9cc9ef588580..009f1dd75dde 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -43,13 +43,14 @@ struct zone *next_zone(struct zone *zone)
>  	return zone;
>  }
>  
> -static inline int zref_in_nodemask(struct zoneref *zref,
> -				   const nodemask_t *nodes)
> +/* A NULL zone terminates the zonelist, so it ends the search. */
> +static inline bool zone_in_nodemask(const struct zone *zone,
> +				    const nodemask_t *nodes)
>  {
>  #ifdef CONFIG_NUMA
> -	return node_isset(zonelist_node_idx(zref), *nodes);
> +	return !zone || node_isset(zone_to_nid(zone), *nodes);
>  #else
> -	return 1;
> +	return true;
>  #endif /* CONFIG_NUMA */
>  }
>  
> @@ -67,7 +68,7 @@ struct zoneref *__next_zones_zonelist(struct zoneref *z,
>  			z++;
>  	else
>  		while (zonelist_zone_idx(z) > highest_zoneidx ||
> -				(zonelist_zone(z) && !zref_in_nodemask(z, nodes)))
> +		       !zone_in_nodemask(READ_ONCE(z->zone), nodes))
>  			z++;
>  
>  	return z;