| Message ID | 4C7F446F.8040303@cn.fujitsu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2010-09-02 at 14:30 +0800, Li Zefan wrote: > Calling task_subsys_state() without holding rcu_read_lock or > cgroup_mutex can cause lockdep warning. > That is not a suitable changelog. Was the warning correct? Is your patch correct? What does RCU protect here and why can we use classid after dropping it. Simply frobbing code to make the warning go away is not good. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Peter Zijlstra <peterz@infradead.org> Date: Thu, 02 Sep 2010 12:38:36 +0200 > On Thu, 2010-09-02 at 14:30 +0800, Li Zefan wrote: >> Calling task_subsys_state() without holding rcu_read_lock or >> cgroup_mutex can cause lockdep warning. >> > > That is not a suitable changelog. > > Was the warning correct? Is your patch correct? What does RCU protect > here and why can we use classid after dropping it. > > Simply frobbing code to make the warning go away is not good. In fact shouldn't this be a rcu_dereference() or similar? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 02, 2010 at 10:05:51AM -0700, David Miller wrote: > From: Peter Zijlstra <peterz@infradead.org> > Date: Thu, 02 Sep 2010 12:38:36 +0200 > > > On Thu, 2010-09-02 at 14:30 +0800, Li Zefan wrote: > >> Calling task_subsys_state() without holding rcu_read_lock or > >> cgroup_mutex can cause lockdep warning. > >> > > > > That is not a suitable changelog. > > > > Was the warning correct? Is your patch correct? What does RCU protect > > here and why can we use classid after dropping it. > > > > Simply frobbing code to make the warning go away is not good. > > In fact shouldn't this be a rcu_dereference() or similar? The rcu_dereference() is there, just buried in task_subsys_state_check() which is called from task_subsys_state(). The code extracts the classid, which is an integer, so the RCU-protected pointer does not leak out of the RCU read-side critical section. So this is OK, but the changelog should indeed say why it is OK. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Peter Zijlstra wrote: > On Thu, 2010-09-02 at 14:30 +0800, Li Zefan wrote: >> Calling task_subsys_state() without holding rcu_read_lock or >> cgroup_mutex can cause lockdep warning. >> > > That is not a suitable changelog. > > Was the warning correct? Is your patch correct? What does RCU protect > here and why can we use classid after dropping it. > > Simply frobbing code to make the warning go away is not good. > > task->cgroups and task->cgroups->subsys[i] are protected by RCU. So we avoid accessing invalid pointers here. This can happen, for example, when you are deref those pointers while someone move @task from one cgroup to another. otoh, there is no lock rule for ->classid. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 02, 2010 at 02:30:07PM +0800, Li Zefan wrote: > Calling task_subsys_state() without holding rcu_read_lock or > cgroup_mutex can cause lockdep warning. > > Reported-by: Dave Jones <davej@redhat.com> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Thanks for catching this.
diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h index dd1fdb8..a4dc5b0 100644 --- a/include/net/cls_cgroup.h +++ b/include/net/cls_cgroup.h @@ -27,11 +27,17 @@ struct cgroup_cls_state #ifdef CONFIG_NET_CLS_CGROUP static inline u32 task_cls_classid(struct task_struct *p) { + int classid; + if (in_interrupt()) return 0; - return container_of(task_subsys_state(p, net_cls_subsys_id), - struct cgroup_cls_state, css)->classid; + rcu_read_lock(); + classid = container_of(task_subsys_state(p, net_cls_subsys_id), + struct cgroup_cls_state, css)->classid; + rcu_read_unlock(); + + return classid; } #else extern int net_cls_subsys_id;
Calling task_subsys_state() without holding rcu_read_lock or cgroup_mutex can cause lockdep warning. Reported-by: Dave Jones <davej@redhat.com> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> --- include/net/cls_cgroup.h | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)