[PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Lai Jiangshan]

It's task->cgroups protected by RCU. and struct css_set.subsys[subsys_id]
is readonly(after init). so we don't need rcu_dereference() for
struct css_set.subsys[subsys_id].

the ways using cgroup_subsys_state() safely:

#1:
rcu_read_lock() / task_lock();
c = cgroup_subsys_state(tsk, id);
use c;
rcu_read_unlock() / task_unlock();


#2: use cgroup_lock() for _current_ task.
cgroup_lock();
c = cgroup_subsys_state(current, id);
use c;
cgroup_unlock();

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1164963..22901ff 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -359,10 +360,15 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
 	return cgrp->subsys[subsys_id];
 }
 
+/* Caller must hold task_lock() or rcu_read_lock() */
 static inline struct cgroup_subsys_state *task_subsys_state(
 	struct task_struct *task, int subsys_id)
 {
-	return rcu_dereference(task->cgroups->subsys[subsys_id]);
+	/*
+	 * ->subsys[subsys_id] are read-only data, so we do not need
+	 * rcu_dereference() for it.
+	 */
+	return rcu_dereference(task->cgroups)->subsys[subsys_id];
 }
 
 static inline struct cgroup* task_cgroup(struct task_struct *task,

Re: [PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Paul Menage]

On Fri, Nov 21, 2008 at 12:49 AM, Lai Jiangshan <laijs-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>
> It's task->cgroups protected by RCU. and struct css_set.subsys[subsys_id]
> is readonly(after init). so we don't need rcu_dereference() for
> struct css_set.subsys[subsys_id].
>
> the ways using cgroup_subsys_state() safely:
>
> #1:
> rcu_read_lock() / task_lock();
> c = cgroup_subsys_state(tsk, id);
> use c;
> rcu_read_unlock() / task_unlock();

You need to qualify that with the fact that if you're just using RCU,
the subsys state may no longer be the state for the task that you're
interested in, since we don't guarantee that the task won't move
directly after you read the state pointer.

>
>
> #2: use cgroup_lock() for _current_ task.
> cgroup_lock();
> c = cgroup_subsys_state(current, id);
> use c;
> cgroup_unlock();

No, if you use cgroup_lock() you can do this for any task.
cgroup_lock() is the cgroups equivalent of the BKL, and definitely
prevents all task movement between groups.
>  static inline struct cgroup_subsys_state *task_subsys_state(
>        struct task_struct *task, int subsys_id)
>  {
> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
> +       /*
> +        * ->subsys[subsys_id] are read-only data, so we do not need
> +        * rcu_dereference() for it.
> +        */
> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>  }

Change looks OK but I think we can lose the additional comment.

Paul

Re: [PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Paul Menage]

On Fri, Nov 21, 2008 at 12:49 AM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
>
> It's task->cgroups protected by RCU. and struct css_set.subsys[subsys_id]
> is readonly(after init). so we don't need rcu_dereference() for
> struct css_set.subsys[subsys_id].
>
> the ways using cgroup_subsys_state() safely:
>
> #1:
> rcu_read_lock() / task_lock();
> c = cgroup_subsys_state(tsk, id);
> use c;
> rcu_read_unlock() / task_unlock();

You need to qualify that with the fact that if you're just using RCU,
the subsys state may no longer be the state for the task that you're
interested in, since we don't guarantee that the task won't move
directly after you read the state pointer.

>
>
> #2: use cgroup_lock() for _current_ task.
> cgroup_lock();
> c = cgroup_subsys_state(current, id);
> use c;
> cgroup_unlock();

No, if you use cgroup_lock() you can do this for any task.
cgroup_lock() is the cgroups equivalent of the BKL, and definitely
prevents all task movement between groups.
>  static inline struct cgroup_subsys_state *task_subsys_state(
>        struct task_struct *task, int subsys_id)
>  {
> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
> +       /*
> +        * ->subsys[subsys_id] are read-only data, so we do not need
> +        * rcu_dereference() for it.
> +        */
> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>  }

Change looks OK but I think we can lose the additional comment.

Paul

Re: [PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Lai Jiangshan]

Paul Menage wrote:
> On Fri, Nov 21, 2008 at 12:49 AM, Lai Jiangshan <laijs-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> It's task->cgroups protected by RCU. and struct css_set.subsys[subsys_id]
>> is readonly(after init). so we don't need rcu_dereference() for
>> struct css_set.subsys[subsys_id].
>>
>> the ways using cgroup_subsys_state() safely:
>>
>> #1:
>> rcu_read_lock() / task_lock();
>> c = cgroup_subsys_state(tsk, id);
>> use c;
>> rcu_read_unlock() / task_unlock();
> 
> You need to qualify that with the fact that if you're just using RCU,
> the subsys state may no longer be the state for the task that you're
> interested in, since we don't guarantee that the task won't move
> directly after you read the state pointer.
> 
>>
>> #2: use cgroup_lock() for _current_ task.
>> cgroup_lock();
>> c = cgroup_subsys_state(current, id);
>> use c;
>> cgroup_unlock();
> 
> No, if you use cgroup_lock() you can do this for any task.
> cgroup_lock() is the cgroups equivalent of the BKL, and definitely
> prevents all task movement between groups.

cgroup_exit() will defeat you.

>>  static inline struct cgroup_subsys_state *task_subsys_state(
>>        struct task_struct *task, int subsys_id)
>>  {
>> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
>> +       /*
>> +        * ->subsys[subsys_id] are read-only data, so we do not need
>> +        * rcu_dereference() for it.
>> +        */
>> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>>  }
> 
> Change looks OK but I think we can lose the additional comment.
> 
> Paul
> 
> 
> 

I just remembered I had deferred Li Zefan's patch.
(I'm also RCU developer, I had been writing CGROUP VS RCU then,
I thought these patches should be sent together, So I deferred his patch)

From: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date: Mon, 25 Aug 2008 11:05:28 +0800
Subject: [PATCH] cgroup: fix wrong rcu_dereference()

It is tsk->cgroups which is protected by RCU.

Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c98dd7c..d911dc7 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -355,7 +355,7 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
 static inline struct cgroup_subsys_state *task_subsys_state(
 	struct task_struct *task, int subsys_id)
 {
-	return rcu_dereference(task->cgroups->subsys[subsys_id]);
+	return rcu_dereference(task->cgroups)->subsys[subsys_id];
 }
 
 static inline struct cgroup* task_cgroup(struct task_struct *task,

Re: [PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Lai Jiangshan]

Paul Menage wrote:
> On Fri, Nov 21, 2008 at 12:49 AM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
>> It's task->cgroups protected by RCU. and struct css_set.subsys[subsys_id]
>> is readonly(after init). so we don't need rcu_dereference() for
>> struct css_set.subsys[subsys_id].
>>
>> the ways using cgroup_subsys_state() safely:
>>
>> #1:
>> rcu_read_lock() / task_lock();
>> c = cgroup_subsys_state(tsk, id);
>> use c;
>> rcu_read_unlock() / task_unlock();
> 
> You need to qualify that with the fact that if you're just using RCU,
> the subsys state may no longer be the state for the task that you're
> interested in, since we don't guarantee that the task won't move
> directly after you read the state pointer.
> 
>>
>> #2: use cgroup_lock() for _current_ task.
>> cgroup_lock();
>> c = cgroup_subsys_state(current, id);
>> use c;
>> cgroup_unlock();
> 
> No, if you use cgroup_lock() you can do this for any task.
> cgroup_lock() is the cgroups equivalent of the BKL, and definitely
> prevents all task movement between groups.

cgroup_exit() will defeat you.

>>  static inline struct cgroup_subsys_state *task_subsys_state(
>>        struct task_struct *task, int subsys_id)
>>  {
>> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
>> +       /*
>> +        * ->subsys[subsys_id] are read-only data, so we do not need
>> +        * rcu_dereference() for it.
>> +        */
>> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>>  }
> 
> Change looks OK but I think we can lose the additional comment.
> 
> Paul
> 
> 
> 

I just remembered I had deferred Li Zefan's patch.
(I'm also RCU developer, I had been writing CGROUP VS RCU then,
I thought these patches should be sent together, So I deferred his patch)

From: Li Zefan <lizf@cn.fujitsu.com>
Date: Mon, 25 Aug 2008 11:05:28 +0800
Subject: [PATCH] cgroup: fix wrong rcu_dereference()

It is tsk->cgroups which is protected by RCU.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/linux/cgroup.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c98dd7c..d911dc7 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -355,7 +355,7 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
 static inline struct cgroup_subsys_state *task_subsys_state(
 	struct task_struct *task, int subsys_id)
 {
-	return rcu_dereference(task->cgroups->subsys[subsys_id]);
+	return rcu_dereference(task->cgroups)->subsys[subsys_id];
 }
 
 static inline struct cgroup* task_cgroup(struct task_struct *task,

Re: [PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Paul Menage]

On Fri, Nov 21, 2008 at 7:22 PM, Lai Jiangshan <laijs-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> No, if you use cgroup_lock() you can do this for any task.
>> cgroup_lock() is the cgroups equivalent of the BKL, and definitely
>> prevents all task movement between groups.
>
> cgroup_exit() will defeat you.

Ah, good point. You should mention that in your description of the
locking rules. But it would be nice if we could get rid of that
restriction.

Paul

>
>>>  static inline struct cgroup_subsys_state *task_subsys_state(
>>>        struct task_struct *task, int subsys_id)
>>>  {
>>> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
>>> +       /*
>>> +        * ->subsys[subsys_id] are read-only data, so we do not need
>>> +        * rcu_dereference() for it.
>>> +        */
>>> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>>>  }
>>
>> Change looks OK but I think we can lose the additional comment.
>>
>> Paul
>>
>>
>>
>
> I just remembered I had deferred Li Zefan's patch.
> (I'm also RCU developer, I had been writing CGROUP VS RCU then,
> I thought these patches should be sent together, So I deferred his patch)
>
> From: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Date: Mon, 25 Aug 2008 11:05:28 +0800
> Subject: [PATCH] cgroup: fix wrong rcu_dereference()
>
> It is tsk->cgroups which is protected by RCU.
>
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> ---
>  include/linux/cgroup.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index c98dd7c..d911dc7 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -355,7 +355,7 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
>  static inline struct cgroup_subsys_state *task_subsys_state(
>        struct task_struct *task, int subsys_id)
>  {
> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>  }
>
>  static inline struct cgroup* task_cgroup(struct task_struct *task,
>
>

Re: [PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Paul Menage]

On Fri, Nov 21, 2008 at 7:22 PM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
>> No, if you use cgroup_lock() you can do this for any task.
>> cgroup_lock() is the cgroups equivalent of the BKL, and definitely
>> prevents all task movement between groups.
>
> cgroup_exit() will defeat you.

Ah, good point. You should mention that in your description of the
locking rules. But it would be nice if we could get rid of that
restriction.

Paul

>
>>>  static inline struct cgroup_subsys_state *task_subsys_state(
>>>        struct task_struct *task, int subsys_id)
>>>  {
>>> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
>>> +       /*
>>> +        * ->subsys[subsys_id] are read-only data, so we do not need
>>> +        * rcu_dereference() for it.
>>> +        */
>>> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>>>  }
>>
>> Change looks OK but I think we can lose the additional comment.
>>
>> Paul
>>
>>
>>
>
> I just remembered I had deferred Li Zefan's patch.
> (I'm also RCU developer, I had been writing CGROUP VS RCU then,
> I thought these patches should be sent together, So I deferred his patch)
>
> From: Li Zefan <lizf@cn.fujitsu.com>
> Date: Mon, 25 Aug 2008 11:05:28 +0800
> Subject: [PATCH] cgroup: fix wrong rcu_dereference()
>
> It is tsk->cgroups which is protected by RCU.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  include/linux/cgroup.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index c98dd7c..d911dc7 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -355,7 +355,7 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
>  static inline struct cgroup_subsys_state *task_subsys_state(
>        struct task_struct *task, int subsys_id)
>  {
> -       return rcu_dereference(task->cgroups->subsys[subsys_id]);
> +       return rcu_dereference(task->cgroups)->subsys[subsys_id];
>  }
>
>  static inline struct cgroup* task_cgroup(struct task_struct *task,
>
>

[PATCH] cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

[Lai Jiangshan]

It's task->cgroups protected by RCU. and struct css_set.subsys[subsys_id]
is readonly(after init). so we don't need rcu_dereference() for
struct css_set.subsys[subsys_id].

the ways using cgroup_subsys_state() safely:

#1:
rcu_read_lock() / task_lock();
c = cgroup_subsys_state(tsk, id);
use c;
rcu_read_unlock() / task_unlock();


#2: use cgroup_lock() for _current_ task.
cgroup_lock();
c = cgroup_subsys_state(current, id);
use c;
cgroup_unlock();

Signed-off-by: Lai Jiangshan <laijs-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1164963..22901ff 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -359,10 +360,15 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
 	return cgrp->subsys[subsys_id];
 }
 
+/* Caller must hold task_lock() or rcu_read_lock() */
 static inline struct cgroup_subsys_state *task_subsys_state(
 	struct task_struct *task, int subsys_id)
 {
-	return rcu_dereference(task->cgroups->subsys[subsys_id]);
+	/*
+	 * ->subsys[subsys_id] are read-only data, so we do not need
+	 * rcu_dereference() for it.
+	 */
+	return rcu_dereference(task->cgroups)->subsys[subsys_id];
 }
 
 static inline struct cgroup* task_cgroup(struct task_struct *task,