linux-kernel - Re: [PATCH 1/7] cgroup: cgroup_subsys->fork() should be called after the task is added to 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: <507F5A81.7010007@huawei.com>
Date:	Thu, 18 Oct 2012 09:25:21 +0800
From:	Li Zefan <lizefan@...wei.com>
To:	Tejun Heo <tj@...nel.org>
CC:	<rjw@...k.pl>, <oleg@...hat.com>, <linux-kernel@...r.kernel.org>,
	<containers@...ts.linux-foundation.org>, <cgroups@...r.kernel.org>,
	<stable@...r.kernel.org>
Subject: Re: [PATCH 1/7] cgroup: cgroup_subsys->fork() should be called after
 the task is added to css_set

于 2012/10/17 16:28, Li Zefan 写道:
> On 2012/10/17 6:28, Tejun Heo wrote:
>> cgroup core has a bug which violates a basic rule about event
>> notifications - when a new entity needs to be added, you add that to
>> the notification list first and then make the new entity conform to
>> the current state.  If done in the reverse order, an event happening
>> inbetween will be lost.
>>
>> cgroup_subsys->fork() is invoked way before the new task is added to
>> the css_set.  Currently, cgroup_freezer is the only user of ->fork()
>> and uses it to make new tasks conform to the current state of the
>> freezer.  If FROZEN state is requested while fork is in progress
>> between cgroup_fork_callbacks() and cgroup_post_fork(), the child
>> could escape freezing - the cgroup isn't frozen when ->fork() is
>> called and the freezer couldn't see the new task on the css_set.
>>
>> This patch moves cgroup_subsys->fork() invocation to
>> cgroup_post_fork() after the new task is added to the css_set.
>> cgroup_fork_callbacks() is removed.
>>
>> Because now a task may be migrated during cgroup_subsys->fork(),
>> freezer_fork() is updated so that it adheres to the usual RCU locking
>> and the rather pointless comment on why locking can be different there
>> is removed (if it doesn't make anything simpler, why even bother?).
>>
> 
> I don't think rcu read section is sufficient. It guarantees the data you're
> accessing is valid, but the data can be new or can be old.
> 
> So a case below is possible:
> 
> in freezer_fork():
> rcu_read_lock();
> freezer = task_freezer(task);
>                                   move task from freezer to freezer2
>                                   which is in FREEZING/FROZEN state
> freezer is in THAWED state,
> nothing to do.
> rcu_read_unlock();
> 

forget about it. The task will be correctly frozen when moving to
another cgroup, so nothing unexpected will happen.

for this patch:

Acked-by: Li Zefan <lizefan@...wei.com>

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