linux-kernel - Re: [patch 1/2] mm: memcg: fix race condition between memcg teardown and swapin

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: <20131205131355.GB16711@dhcp22.suse.cz>
Date:	Thu, 5 Dec 2013 14:13:55 +0100
From:	Michal Hocko <mhocko@...e.cz>
To:	Johannes Weiner <hannes@...xchg.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	David Rientjes <rientjes@...gle.com>, linux-mm@...ck.org,
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 1/2] mm: memcg: fix race condition between memcg teardown
 and swapin

On Wed 04-12-13 17:45:13, Johannes Weiner wrote:
> There is a race condition between a memcg being torn down and a swapin
> triggered from a different memcg of a page that was recorded to belong
> to the exiting memcg on swapout (with CONFIG_MEMCG_SWAP extension).
> The result is unreclaimable pages pointing to dead memcgs, which can
> lead to anything from endless loops in later memcg teardown (the page
> is charged to all hierarchical parents but is not on any LRU list) or
> crashes from following the dangling memcg pointer.
> 
> Memcgs with tasks in them can not be torn down and usually charges
> don't show up in memcgs without tasks.  Swapin with the
> CONFIG_MEMCG_SWAP extension is the notable exception because it
> charges the cgroup that was recorded as owner during swapout, which
> may be empty and in the process of being torn down when a task in
> another memcg triggers the swapin:
> 
>   teardown:                 swapin:
> 
>                             lookup_swap_cgroup_id()
>                             rcu_read_lock()
>                             mem_cgroup_lookup()
>                             css_tryget()
>                             rcu_read_unlock()
>   disable css_tryget()
>   call_rcu()
>     offline_css()
>       reparent_charges()
>                             res_counter_charge() (hierarchical!)
>                             css_put()
>                               css_free()
>                             pc->mem_cgroup = dead memcg
>                             add page to dead lru
> 
> Add a final reparenting step into css_free() to make sure any such
> raced charges are moved out of the memcg before it's finally freed.
> 
> In the longer term it would be cleaner to have the css_tryget() and
> the res_counter charge under the same RCU lock section so that the
> charge reparenting is deferred until the last charge whose tryget
> succeeded is visible.  But this will require more invasive changes
> that will be harder to evaluate and backport into stable, so better
> defer them to a separate change set.
> 
> Signed-off-by: Johannes Weiner <hannes@...xchg.org>
> Cc: <stable@...nel.org>

I have a patch to fix this properly but I didn't get to test it yet. I
will post it sometimes soon hopefully.

For the meantime
Acked-by: Michal Hocko <mhocko@...e.cz>

> ---
>  mm/memcontrol.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e3aff0175d4c..f6a63f5b3827 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6355,6 +6355,42 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
>  static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> +	/*
> +	 * XXX: css_offline() would be where we should reparent all
> +	 * memory to prepare the cgroup for destruction.  However,
> +	 * memcg does not do css_tryget() and res_counter charging
> +	 * under the same RCU lock region, which means that charging
> +	 * could race with offlining.  Offlining only happens to
> +	 * cgroups with no tasks in them but charges can show up
> +	 * without any tasks from the swapin path when the target
> +	 * memcg is looked up from the swapout record and not from the
> +	 * current task as it usually is.  A race like this can leak
> +	 * charges and put pages with stale cgroup pointers into
> +	 * circulation:
> +	 *
> +	 * #0                        #1
> +	 *                           lookup_swap_cgroup_id()
> +	 *                           rcu_read_lock()
> +	 *                           mem_cgroup_lookup()
> +	 *                           css_tryget()
> +	 *                           rcu_read_unlock()
> +	 * disable css_tryget()
> +	 * call_rcu()
> +	 *   offline_css()
> +	 *     reparent_charges()
> +	 *                           res_counter_charge()
> +	 *                           css_put()
> +	 *                             css_free()
> +	 *                           pc->mem_cgroup = dead memcg
> +	 *                           add page to lru
> +	 *
> +	 * The bulk of the charges are still moved in offline_css() to
> +	 * avoid pinning a lot of pages in case a long-term reference
> +	 * like a swapout record is deferring the css_free() to long
> +	 * after offlining.  But this makes sure we catch any charges
> +	 * made after offlining:
> +	 */
> +	mem_cgroup_reparent_charges(memcg);
>  
>  	memcg_destroy_kmem(memcg);
>  	__mem_cgroup_free(memcg);
> -- 
> 1.8.4.2
> 

-- 
Michal Hocko
SUSE Labs
--
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/