MIME-Version: 1.0
References: <20211206033338.743270-1-npache@redhat.com> <20211206033338.743270-3-npache@redhat.com>
In-Reply-To: <20211206033338.743270-3-npache@redhat.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Mon, 6 Dec 2021 10:45:19 -0800
Message-ID: <CAHbLzkrwXYxZz37g-4aW1Gbs8sxTB72X6rH8DM+V+WKuwEg2fA@mail.gmail.com>
Subject: Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on
 offlined nodes
To:     Nico Pache <npache@redhat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Shakeel Butt <shakeelb@google.com>,
        Kirill Tkhai <ktkhai@virtuozzo.com>,
        Roman Gushchin <guro@fb.com>, Vlastimil Babka <vbabka@suse.cz>,
        Vladimir Davydov <vdavydov.dev@gmail.com>, raquini@redhat.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>