Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on offlined nodes

[David Hildenbrand <david@redhat.com>]

On 06.12.21 04:33, Nico Pache wrote:
> We have run into a panic caused by a shrinker allocation being attempted
> on an offlined node.
> 
> Our crash analysis has determined that the issue originates from trying
> to allocate pages on an offlined node in expand_one_shrinker_info. This
> function makes the incorrect assumption that we can allocate on any node.
> To correct this we make sure we only itterate over online nodes.
> 
> This assumption can lead to an incorrect address being assigned to ac->zonelist
> in the following callchain:
> 	__alloc_pages
> 	-> prepare_alloc_pages
> 	 -> node_zonelist
> 
> static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
> {
>         return NODE_DATA(nid)->node_zonelists + gfp_zonelist(flags);
> }
> if the node is not online the return of node_zonelist will evaluate to a
> invalid pointer of 0x00000 + offset_of(node_zonelists) + (1|0)
> 
> This address is then dereferenced further down the callchain in:
> 	prepare_alloc_pages
> 	-> first_zones_zonelist
>   	 -> next_zones_zonelist
> 	  -> zonelist_zone_idx
> 
> static inline int zonelist_zone_idx(struct zoneref *zoneref)
> {
>         return zoneref->zone_idx;
> }
> 
> Leading the system to panic.
> 
> We also correct this behavior in alloc_shrinker_info, free_shrinker_info,
> and reparent_shrinker_deferred.
> 
> Fixes: 2bfd36374edd ("mm: vmscan: consolidate shrinker_maps handling code")
> Fixes: 0a4465d34028 ("mm, memcg: assign memcg-aware shrinkers bitmap to memcg")
> Signed-off-by: Nico Pache <npache@redhat.com>
> ---
>  mm/vmscan.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index fb9584641ac7..731564b61e3f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -221,7 +221,7 @@ static int expand_one_shrinker_info(struct mem_cgroup *memcg,
>  	int nid;
>  	int size = map_size + defer_size;
>  
> -	for_each_node(nid) {
> +	for_each_online_node(nid) {
>  		pn = memcg->nodeinfo[nid];
>  		old = shrinker_info_protected(memcg, nid);
>  		/* Not yet online memcg */
> @@ -256,7 +256,7 @@ void free_shrinker_info(struct mem_cgroup *memcg)
>  	struct shrinker_info *info;
>  	int nid;
>  
> -	for_each_node(nid) {
> +	for_each_online_node(nid) {
>  		pn = memcg->nodeinfo[nid];
>  		info = rcu_dereference_protected(pn->shrinker_info, true);
>  		kvfree(info);
> @@ -274,7 +274,7 @@ int alloc_shrinker_info(struct mem_cgroup *memcg)
>  	map_size = shrinker_map_size(shrinker_nr_max);
>  	defer_size = shrinker_defer_size(shrinker_nr_max);
>  	size = map_size + defer_size;
> -	for_each_node(nid) {
> +	for_each_online_node(nid) {
>  		info = kvzalloc_node(sizeof(*info) + size, GFP_KERNEL, nid);
>  		if (!info) {
>  			free_shrinker_info(memcg);
> @@ -417,7 +417,7 @@ void reparent_shrinker_deferred(struct mem_cgroup *memcg)
>  
>  	/* Prevent from concurrent shrinker_info expand */
>  	down_read(&shrinker_rwsem);
> -	for_each_node(nid) {
> +	for_each_online_node(nid) {
>  		child_info = shrinker_info_protected(memcg, nid);
>  		parent_info = shrinker_info_protected(parent, nid);
>  		for (i = 0; i < shrinker_nr_max; i++) {
> 

What happens on memory/cpu hotplug, resulting in a new node getting
onlined? Will the data structures get allocated and the data get
properly set up?

-- 
Thanks,

David / dhildenb