On Sun, Dec 5, 2021 at 7:34 PM Nico Pache <npache@xxxxxxxxxx> 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")
I think the correct fix tag should be: 86daf94efb11 ("mm/memcontrol.c:
allocate shrinker_map on appropriate NUMA node") regardless of how we
will fix this problem.
> Signed-off-by: Nico Pache <npache@xxxxxxxxxx>
> ---
> 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++) {
> --
> 2.33.1
>
