linux-kernel - Re: [RFC][PATCH 0/2] cgroup: Fix some races against 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]

Date:	Fri, 17 Feb 2012 13:41:21 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
CC:	Tejun Heo <tj@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	Mandeep Singh Baines <msb@...omium.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [RFC][PATCH 0/2] cgroup: Fix some races against css_set task
 links

sorry for the delayed reply. I had been off for quite a few days.

Frederic Weisbecker wrote:
> Hi,
> 
> This is trying to fix some races in the way to link all the tasks to
> the css_set links. I hope some people can have a look at this, especially
> as I'm definetly not an SMP ordering expert.

me neither.

> 
> To give you the big picture, as long as nobody never calls
> cgroup_iter_start(), we don't link the tasks to their css_set (this 'link'
> is a simple list_add()).
> 
> But once somebody calls cgroup_iter_start(), we call
> cgroup_enable_task_cg_lists() that grossly does this:
> 
> cgroup_enable_task_cg_lists() {
> 	use_task_set_css_links = 1;
> 	do_each_thread(g, p) {
> 		link p to css_set
> 	} while_each_thread(g, p);
> }
> 
> But this links only existing tasks, we also need to link all the tasks
> that will be created later, this is what does cgroup_post_fork():
> 
> cgroup_post_fork() {
> 	if (use_task_set_css_links)
> 		link p to css_set
> }
> 
> So we have some races here:
> 
> - cgroup_enable_task_cg_lists() iterates over the tasklist
>   without protection. The comments are advertizing we are using RCU
>   but we don't. And in fact RCU doesn't yet protect against
>   while_each_thread().
> 
> - Moreover with RCU there is a risk that we iterate the tasklist but
>   we don't immediately see all the last updates that happened. For
>   example if a task forks and passes cgroup_post_fork() while
>   use_task_set_css_links = 0 then another CPU calling
>   cgroup_enable_task_cg_list() can miss the new child while walking the
>   tasklist with RCU as it doesn't appear immediately.
> 
> - There is no ordering constraint on use_task_set_css_links read/write
>   against the tasklist traversal and modification. cgroup_post_fork()
>   may deal with a stale value.
> 
> The second patch of the series is a proposal to fix the three above
> points. Tell me what you think.
> 

The patch looks good to me.

As cgroup_enable_task_cg_lists() is an one-off function, won't be
called more than once, so there's little chance it can happen
in reality, so should be ok to queue it for 3.4.

> Thanks.
> 
> Frederic Weisbecker (2):
>   cgroup: Remove wrong comment on cgroup_enable_task_cg_list()
>   cgroup: Walk task list under tasklist_lock in
>     cgroup_enable_task_cg_list
> 

for both patches

Acked-by: Li Zefan <lizf@...fujitsu.com>

>  kernel/cgroup.c |   23 ++++++++++++++++++++---
>  1 files changed, 20 insertions(+), 3 deletions(-)
> 
--
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/