Messages in this thread | | | Date | Mon, 6 Dec 2021 09:32:47 +0100 | Subject | Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on offlined nodes | From | David Hildenbrand <> |
| |
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
| |