lkml.org 
[lkml]   [2021]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on offlined nodes
    On Sun, Dec 5, 2021 at 7:34 PM Nico Pache <npache@redhat.com> 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@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++) {
    > --
    > 2.33.1
    >

    \
     
     \ /
      Last update: 2021-12-06 19:47    [W:5.483 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site