cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

PATCH In review Lai Jiangshan 4 participants 7 messages Nov 21, 2008 – Nov 24, 2008 3 messages not in the archive cgroup

lore

Share & tools

lore

Lai Jiangshan

author

2008-11-21 08:54 UTC #0 lore raw

PATCH 1/1 cgroups: fix incorrect using rcu_dereference() in cgroup_subsys_state()

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 <[email protected]>

---

include/linux/cgroup.h +6 -1

index 1164963..22901ff 100644
@@ -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,

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul Menage

2008-11-21 18:07 UTC #2 lore raw

Quoted 5 lines · 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].

Quoted 7 lines

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.

Quoted 7 lines

#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.

include/linux/cgroup.h static inline struct cgroup_subsys_state *cgroup_subsys_state( · line 372

Quoted 10 lines

static inline struct cgroup_subsys_state *task_subsys_state(
struct task_struct *task, int subsys_id)
{

include/linux/cgroup.h +5 -1

-       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

Paul Menage

2008-11-21 18:08 UTC #3 lore raw

In reply to Lai Jiangshan

Quoted 5 lines · 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].

Quoted 7 lines

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();

Quoted 7 lines

#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.

include/linux/cgroup.h static inline struct cgroup_subsys_state *cgroup_subsys_state( · line 372

Quoted 10 lines

static inline struct cgroup_subsys_state *task_subsys_state(
struct task_struct *task, int subsys_id)
{

include/linux/cgroup.h +5 -1

-       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

A message that is not in the archive (sent off-list or not yet received): 6599ad830811211007s43f0c683s9d56e0aed3af938-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Lai Jiangshan

2008-11-22 03:22 UTC #5 lore raw

Quoted 25 lines · Paul Menage

Quoted 11 lines · 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();
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.
Quoted 6 lines
#2: use cgroup_lock() for _current_ task.
cgroup_lock();
c = cgroup_subsys_state(current, id);
use c;
cgroup_unlock();

Quoted 3 lines

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.

Quoted 11 lines

Review
Quoted 10 lines
static inline struct cgroup_subsys_state *task_subsys_state(
struct task_struct *task, int subsys_id)
{
(quoted diff) +5 -1
-       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];
 }

Quoted 6 lines

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 <[email protected]>
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 <[email protected]>

---
 include/linux/cgroup.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/linux/cgroup.h +1 -1

index c98dd7c..d911dc7 100644
@@ -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,

Lai Jiangshan

author

2008-11-22 03:25 UTC #6 lore raw

In reply to Paul Menage

Quoted 25 lines · Paul Menage

Quoted 11 lines · 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();
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.
Quoted 6 lines
#2: use cgroup_lock() for _current_ task.
cgroup_lock();
c = cgroup_subsys_state(current, id);
use c;
cgroup_unlock();

Quoted 3 lines

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.

Quoted 11 lines

Review
Quoted 10 lines
static inline struct cgroup_subsys_state *task_subsys_state(
struct task_struct *task, int subsys_id)
{
(quoted diff) +5 -1
-       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];
 }

Quoted 6 lines

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 <[email protected]>
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 <[email protected]>

---
 include/linux/cgroup.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/linux/cgroup.h +1 -1

index c98dd7c..d911dc7 100644
@@ -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,

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul Menage

2008-11-24 20:45 UTC #8 lore raw

Quoted 5 lines · Lai Jiangshan

Quoted 3 lines
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

Quoted 49 lines

Quoted 12 lines
Review
Quoted 11 lines
static inline struct cgroup_subsys_state *task_subsys_state(
struct task_struct *task, int subsys_id)
{
(quoted diff) +5 -1
-       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];
 }
Quoted 6 lines
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 <[email protected]>
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 <[email protected]>
---
 include/linux/cgroup.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/cgroup.h +1 -1
index c98dd7c..d911dc7 100644
@@ -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,

Paul Menage

2008-11-24 20:46 UTC #9 lore raw

In reply to Lai Jiangshan

Quoted 5 lines · Lai Jiangshan

Quoted 3 lines
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

Quoted 49 lines

Quoted 12 lines
Review
Quoted 11 lines
static inline struct cgroup_subsys_state *task_subsys_state(
struct task_struct *task, int subsys_id)
{
(quoted diff) +5 -1
-       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];
 }
Quoted 6 lines
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 <[email protected]>
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 <[email protected]>
---
 include/linux/cgroup.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/cgroup.h +1 -1
index c98dd7c..d911dc7 100644
@@ -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,