Date:   Mon, 6 Dec 2021 10:24:14 +0100
From:   Michal Hocko <mhocko@suse.com>
To:     Nico Pache <npache@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, shakeelb@google.com,
        ktkhai@virtuozzo.com, shy828301@gmail.com, guro@fb.com,
        vbabka@suse.cz, vdavydov.dev@gmail.com, raquini@redhat.com,
        David Hildenbrand <david@redhat.com>
Subject: Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on
 offlined nodes
Message-ID: <Ya3WvmwzrKfnZsy/@dhcp22.suse.cz>
References: <20211206033338.743270-1-npache@redhat.com>
 <20211206033338.743270-3-npache@redhat.com>
 <Ya3WcYKcej8XEI0W@dhcp22.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ya3WcYKcej8XEI0W@dhcp22.suse.cz>
Precedence: bulk

[Cc David. I have only now noticed he has replied to this thread already
 pointing out the offline->online case]

On Mon 06-12-21 10:23:00, Michal Hocko wrote:
> On Sun 05-12-21 22:33:38, 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.
> 
> Thanks for the analysis! Please also add an oops report so that this is
> easier to search for. It would be also interesting to see specifics
> about the issue. Why was the specific node !online in the first place?
> What architecture was this on?
> 
> > 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")
> 
> Normally I would split the fix as it is fixing two issues one introduced
> in 4.19 the other in 5.13.
> 
> > 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
> 
> This doesn't seen complete. Slab shrinkers are used in the reclaim
> context. Previously offline nodes could be onlined later and this would
> lead to NULL ptr because there is no hook to allocate new shrinker
> infos. This would be also really impractical because this would have to
> update all existing memcgs...
> 
> To be completely honest I am not really sure this is a practical problem
> because some architectures allocate (aka make online) all possible nodes
> reported by the platform. There are major inconsistencies there. Maybe
> that should be unified, so that problems like this one do not really
> have to add a complexity to the code.
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs