linux-kernel - Re: [PATCH] memcg: don't do cleanup manually if mem_cgroup_css

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: <515ACD7F.3070009@parallels.com>
Date:	Tue, 2 Apr 2013 16:22:23 +0400
From:	Glauber Costa <glommer@...allels.com>
To:	Michal Hocko <mhocko@...e.cz>
CC:	Li Zefan <lizefan@...wei.com>,
	Johannes Weiner <hannes@...xchg.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Cgroups <cgroups@...r.kernel.org>, <linux-mm@...ck.org>
Subject: Re: [PATCH] memcg: don't do cleanup manually if mem_cgroup_css_online()
 fails

On 04/02/2013 04:16 PM, Michal Hocko wrote:
> On Tue 02-04-13 15:35:28, Li Zefan wrote:
> [...]
>> @@ -6247,16 +6247,7 @@ mem_cgroup_css_online(struct cgroup *cont)
>>  
>>  	error = memcg_init_kmem(memcg, &mem_cgroup_subsys);
>>  	mutex_unlock(&memcg_create_mutex);
>> -	if (error) {
>> -		/*
>> -		 * We call put now because our (and parent's) refcnts
>> -		 * are already in place. mem_cgroup_put() will internally
>> -		 * call __mem_cgroup_free, so return directly
>> -		 */
>> -		mem_cgroup_put(memcg);
>> -		if (parent->use_hierarchy)
>> -			mem_cgroup_put(parent);
>> -	}
>> +
>>  	return error;
>>  }
> 
> The mem_cgroup_put(parent) part is incorrect because mem_cgroup_put goes
> up the hierarchy already but I do not think mem_cgroup_put(memcg) should
> go away as well. Who is going to free the last reference then?
> 
> Maybe I am missing something but we have:
> cgroup_create
>   css = ss->css_alloc(cgrp)
>     mem_cgroup_css_alloc
>       atomic_set(&memcg->refcnt, 1)
>   online_css(ss, cgrp)
>     mem_cgroup_css_online
>       error = memcg_init_kmem		# fails
>   goto err_destroy
> err_destroy:
>   cgroup_destroy_locked(cgrp)
>     offline_css
>       mem_cgroup_css_offline
> 
> no mem_cgroup_put on the way.
> 

static void mem_cgroup_css_free(struct cgroup *cont)
{
        struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);

        kmem_cgroup_destroy(memcg);

        mem_cgroup_put(memcg);
}

kernel/cgroup.c:
err_free_all:
        for_each_subsys(root, ss) {
                if (cgrp->subsys[ss->subsys_id])
                        ss->css_free(cgrp);
        }

As I've said, this error path predates kmemcg for a long time. I wasn't
still able to identify precisely why it was working - assuming it was
indeed working (I remember having tested it with handcrafted manual
errors, but memory can always fail...).

But our best theory so far is that the cgroup rework started calling the
free function that wasn't called before.

--
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/