lkml.org 
[lkml]   [2012]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@cn.fujitsu.com>

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


\
 
 \ /
  Last update: 2012-02-17 06:41    [W:0.084 / U:1.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site