Skip to contentSkip to conversation

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

PATCH RESEND In review Lai Jiangshan 3 participants 3 messages Jan 4, 2011 – Jan 5, 2011 cgroup

lore
Share & tools
lore

Lai Jiangshan

author
#0 lore raw

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

From: Li Zefan <[email protected]>
Date: Mon, 25 Aug 2008 11:05:28 +0800
(Original) Subject: [PATCH] cgroup: fix wrong rcu_dereference()

It is tsk->cgroups which is protected by RCU, not ->subsys[subsys_id].

laijs: updated it(the surrounding code have been changed since these two years).

Signed-off-by Li Zefan <[email protected]>
Signed-off-by Lai Jiangshan <[email protected]>

---

include/linux/cgroup.h +3 -2
index ed4ba11..a798814 100644
@@ -535,10 +535,11 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
  * cgroup_subsys::attach() methods.
  */
 #define task_subsys_state_check(task, subsys_id, __c)			\
-	rcu_dereference_check(task->cgroups->subsys[subsys_id],		\
+	rcu_dereference_check(task->cgroups,				\
 			      rcu_read_lock_held() ||			\
 			      lockdep_is_held(&task->alloc_lock) ||	\
-			      cgroup_lock_is_held() || (__c))
+			      cgroup_lock_is_held() ||			\
+			      (__c))->subsys[subsys_id]
 
 static inline struct cgroup_subsys_state *
 task_subsys_state(struct task_struct *task, int subsys_id)
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · Lai Jiangshan

From: Li Zefan <[email protected]>
Date: Mon, 25 Aug 2008 11:05:28 +0800
(Original) Subject: [PATCH] cgroup: fix wrong rcu_dereference()

It is tsk->cgroups which is protected by RCU, not ->subsys[subsys_id].

laijs: updated it(the surrounding code have been changed since these two years).

This looks plausible to me, assuming that the cgroups guys are OK with it.

One requested change: could you please delete the rcu_read_lock_held()? This is now implied by rcu_dereference_check(). With that:

Acked-by Paul E. McKenney <[email protected]>
Quoted 21 lines
Signed-off-by Li Zefan <[email protected]>
Signed-off-by Lai Jiangshan <[email protected]>

---

include/linux/cgroup.h +3 -2
index ed4ba11..a798814 100644
@@ -535,10 +535,11 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
  * cgroup_subsys::attach() methods.
  */
 #define task_subsys_state_check(task, subsys_id, __c)			\
-	rcu_dereference_check(task->cgroups->subsys[subsys_id],		\
+	rcu_dereference_check(task->cgroups,				\
 			      rcu_read_lock_held() ||			\
 			      lockdep_is_held(&task->alloc_lock) ||	\
-			      cgroup_lock_is_held() || (__c))
+			      cgroup_lock_is_held() ||			\
+			      (__c))->subsys[subsys_id]
 
 static inline struct cgroup_subsys_state *
 task_subsys_state(struct task_struct *task, int subsys_id)
#2 lore raw
In reply to Lai Jiangshan
Quoted 16 lines · Lai Jiangshan

From: Li Zefan <[email protected]>
Date: Mon, 25 Aug 2008 11:05:28 +0800
(Original) Subject: [PATCH] cgroup: fix wrong rcu_dereference()

It is tsk->cgroups which is protected by RCU, not ->subsys[subsys_id].

laijs: updated it(the surrounding code have been changed since these two years).

Signed-off-by Li Zefan <[email protected]>
Signed-off-by Lai Jiangshan <[email protected]>

---

include/linux/cgroup.h +0 -0
index ed4ba11..a798814 100644

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

Quoted 10 lines
include/linux/cgroup.h +3 -2
@@ -535,10 +535,11 @@ static inline struct cgroup_subsys_state *cgroup_subsys_state(
  * cgroup_subsys::attach() methods.
  */
  #define task_subsys_state_check(task, subsys_id, __c)                  \
-       rcu_dereference_check(task->cgroups->subsys[subsys_id],         \
+       rcu_dereference_check(task->cgroups,                            \
                              rcu_read_lock_held() ||                   \
                              lockdep_is_held(&task->alloc_lock) ||     \
-                             cgroup_lock_is_held() || (__c))
+                             cgroup_lock_is_held() ||                  \
+                             (__c))->subsys[subsys_id]

Can you reformat this a bit? It took a fair bit of staring to realize that (__c))->subsys[subsys_id] wasn't trying to dereference __c.

Maybe make it:

#define task_subsys_state_check(task, subsys_id, __c) ({ \
struct css_set *_css = rcu_dereference_check(...); \
_css->subsys[subsys_id]; \
})

Paul