linux-kernel - Re: [PATCH] mm/page_alloc: detect allocation forbidden by cpuset and bail out early

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210910112953.GB54659@shbuild999.sh.intel.com>
Date:   Fri, 10 Sep 2021 19:29:53 +0800
From:   Feng Tang <feng.tang@...el.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Vlastimil Babka <vbabka@...e.cz>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/page_alloc: detect allocation forbidden by cpuset and
 bail out early

On Fri, Sep 10, 2021 at 12:35:02PM +0200, Michal Hocko wrote:
> I would squash the two changes into a single patch.
> 
> On Fri 10-09-21 17:21:32, Feng Tang wrote:
> [...]
> > +/*
> > + * Check if there has been insane configurations. E.g. there was usages
> > + * which binds a docker OS to memory nodes with only movable zones, which
> > + * causes system to behave abnormally, as the usage triggers many innocent
> > + * processes get oom-killed.
> 
> I would go with more specifics here. What about
> 
> /*
>  * This will get enabled whenever a cpuset configuration is considered
>  * unsupportable in general. E.g. movable only node which cannot satisfy
>  * any non movable allocations (see update_nodemask).
>  * Page allocator needs to make additional checks for those
>  * configurations and this check is meant to guard those checks without
>  * any overhead for sane configurations.
>  */

will use this, thanks

> > + */
> > +static inline bool cpusets_insane_config(void)
> > +{
> > +	return static_branch_unlikely(&cpusets_insane_config_key);
> > +}
> > +
> >  extern int cpuset_init(void);
> >  extern void cpuset_init_smp(void);
> >  extern void cpuset_force_rebuild(void);
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 6a1d79d..c3f5527 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1116,6 +1116,20 @@ extern struct zone *next_zone(struct zone *zone);
> >  			; /* do nothing */		\
> >  		else
> >  
> > +/* Whether the 'nodes' are all movable nodes */
> > +static inline bool movable_only_nodes(nodemask_t *nodes)
> > +{
> > +	struct zone *zone;
> > +
> > +	for_each_populated_zone(zone) {
> > +		if (zone_idx(zone) != ZONE_MOVABLE &&
> > +			node_isset(zone_to_nid(zone), *nodes))
> > +			return false;
> > +	}
> > +
> > +	return true;
> 
> Sorry I didn't really get to read this previously. The implementation
> works but I find it harder to read than really necessary. Why don't you
> use first_zones_zonelist here as well?

The concern I had was which zonelist to use, local node or the first node
of nodemask's node_zonelists[ZONELIST_FALLBACK], and the node_zonelists[]
is initialized in node init time, I'm not sure if all nodes's zone will
be showed in it (though it is 'yes' for current code).

Also, I tried another logic (a new 'for_each_populated_zone_nodes' macro)
	for_each_populated_zone_nodes(zone, node, *nodes) {
		if (zone_idx(zone) != ZONE_MOVABLE)
 			return false;
 	}

> > +}
> > +
> >  static inline struct zone *zonelist_zone(struct zoneref *zoneref)
> >  {
> >  	return zoneref->zone;
> > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> > index df1ccf4..e0cb12e 100644
> > --- a/kernel/cgroup/cpuset.c
> > +++ b/kernel/cgroup/cpuset.c
> > @@ -69,6 +69,13 @@
> >  DEFINE_STATIC_KEY_FALSE(cpusets_pre_enable_key);
> >  DEFINE_STATIC_KEY_FALSE(cpusets_enabled_key);
> >  
> > +/*
> > + * There could be abnormal cpuset configurations for cpu or memory
> > + * node binding, add this key to provide a quick low-cost judgement
> > + * of the situation.
> > + */
> > +DEFINE_STATIC_KEY_FALSE(cpusets_insane_config_key);
> > +
> >  /* See "Frequency meter" comments, below. */
> >  
> >  struct fmeter {
> > @@ -1868,6 +1875,12 @@ static int update_nodemask(struct cpuset *cs, struct cpuset *trialcs,
> >  	if (retval < 0)
> >  		goto done;
> >  
> > +	if (movable_only_nodes(&trialcs->mems_allowed)) {
> > +		static_branch_enable(&cpusets_insane_config_key);
> > +		pr_info("cpuset: See abornal binding to movable nodes only(nmask=%*pbl)\n",
> > +			nodemask_pr_args(&trialcs->mems_allowed));
> 
> This doesn't sound very useful for admins IMHO. It is not clear what the
> problem is and how to deal with it. What about
> 		pr_into("Unsupported (movable nodes only) cpuset configuration detected! Cpuset allocations might fail even with a lot of memory available.");

Yep, this is better!

Thanks,
Feng