* [PATCH] cls_cgroup: Fix rcu lockdep warning
@ 2010-09-02 6:30 Li Zefan
2010-09-02 10:38 ` Peter Zijlstra
2010-09-03 4:52 ` Herbert Xu
0 siblings, 2 replies; 6+ messages in thread
From: Li Zefan @ 2010-09-02 6:30 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Dave Jones, netdev, LKML
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(-)
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;
--
1.6.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] cls_cgroup: Fix rcu lockdep warning
2010-09-02 6:30 [PATCH] cls_cgroup: Fix rcu lockdep warning Li Zefan
@ 2010-09-02 10:38 ` Peter Zijlstra
2010-09-02 17:05 ` David Miller
2010-09-03 1:10 ` Li Zefan
2010-09-03 4:52 ` Herbert Xu
1 sibling, 2 replies; 6+ messages in thread
From: Peter Zijlstra @ 2010-09-02 10:38 UTC (permalink / raw)
To: Li Zefan
Cc: David Miller, Herbert Xu, Dave Jones, netdev, LKML,
Paul E. McKenney
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cls_cgroup: Fix rcu lockdep warning
2010-09-02 10:38 ` Peter Zijlstra
@ 2010-09-02 17:05 ` David Miller
2010-09-02 17:16 ` Paul E. McKenney
2010-09-03 1:10 ` Li Zefan
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2010-09-02 17:05 UTC (permalink / raw)
To: peterz; +Cc: lizf, herbert, davej, netdev, linux-kernel, paulmck
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cls_cgroup: Fix rcu lockdep warning
2010-09-02 17:05 ` David Miller
@ 2010-09-02 17:16 ` Paul E. McKenney
0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2010-09-02 17:16 UTC (permalink / raw)
To: David Miller; +Cc: peterz, lizf, herbert, davej, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cls_cgroup: Fix rcu lockdep warning
2010-09-02 10:38 ` Peter Zijlstra
2010-09-02 17:05 ` David Miller
@ 2010-09-03 1:10 ` Li Zefan
1 sibling, 0 replies; 6+ messages in thread
From: Li Zefan @ 2010-09-03 1:10 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Miller, Herbert Xu, Dave Jones, netdev, LKML,
Paul E. McKenney
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cls_cgroup: Fix rcu lockdep warning
2010-09-02 6:30 [PATCH] cls_cgroup: Fix rcu lockdep warning Li Zefan
2010-09-02 10:38 ` Peter Zijlstra
@ 2010-09-03 4:52 ` Herbert Xu
1 sibling, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2010-09-03 4:52 UTC (permalink / raw)
To: Li Zefan; +Cc: David Miller, Dave Jones, netdev, LKML
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.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-03 4:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-02 6:30 [PATCH] cls_cgroup: Fix rcu lockdep warning Li Zefan
2010-09-02 10:38 ` Peter Zijlstra
2010-09-02 17:05 ` David Miller
2010-09-02 17:16 ` Paul E. McKenney
2010-09-03 1:10 ` Li Zefan
2010-09-03 4:52 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).