linux-kernel - Re: [PATCH -v2 4/6] memcg: make sure that memcg is not offline when charging

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: <20140204162939.GP6963@cmpxchg.org>
Date:	Tue, 4 Feb 2014 11:29:39 -0500
From:	Johannes Weiner <hannes@...xchg.org>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org
Subject: Re: [PATCH -v2 4/6] memcg: make sure that memcg is not offline when
 charging

On Tue, Feb 04, 2014 at 02:28:58PM +0100, Michal Hocko wrote:
> The current charge path might race with memcg offlining because holding
> css reference doesn't neither prevent from task move to a different
> group nor stop css offline. When a charging task is the last one in the
> group and it is moved to a different group in the middle of the charge
> the old memcg might get offlined. As a result res counter might be
> charged after mem_cgroup_reparent_charges (called from memcg css_offline
> callback) and so the charge would never be freed. This has been worked
> around by 96f1c58d8534 (mm: memcg: fix race condition between memcg
> teardown and swapin) which tries to catch such a leaked charges later
> during css_free. It is more optimal to heal this race in the long term
> though.
> 
> In order to make this raceless we have to check that the memcg is online
> and res_counter_charge in the same RCU read section. The online check can
> be done simply by calling css_tryget & css_put which are now wrapped
> into mem_cgroup_is_online helper.
> 
> Callers are then updated to retry with a new memcg which is associated
> with the current mm. There always has to be a valid memcg encountered
> sooner or later because task had to be moved to a valid and online
> cgroup.
> 
> The only exception is mem_cgroup_do_precharge which should never see
> this race because it is called from cgroup {can_}attach callbacks and so
> the whole cgroup cannot go away.
> 
> Signed-off-by: Michal Hocko <mhocko@...e.cz>
> ---
>  mm/memcontrol.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 66 insertions(+), 4 deletions(-)

Maybe we should remove the XXX if it makes you think we should change
the current situation by any means necessary.  This patch is not an
improvement.

I put the XXX there so that we one day maybe refactor the code in a
clean fashion where try_get_mem_cgroup_from_whatever() is in the same
rcu section as the first charge attempt.  On failure, reclaim, and do
the lookup again.

Also, this problem only exists on swapin, where the memcg is looked up
from an auxilliary data structure and not the current task, so maybe
that would be an angle to look for a clean solution.

Either way, the problem is currently fixed with a *oneliner*.  Unless
the alternative solution is inherent in a clean rework of the code to
match cgroup core lifetime management, I don't see any reason to move
away from the status quo.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/