RCU: don't turn off lockdep when find suspicious rcu_dereference_check() usage
in [Kernel]
|
Prev: [PATCH] rmap: anon_vma_prepare() can leak anon_vma_chain
Next: [PATCH] x86/PCI: parse additional host bridge window resource types
From: Miles Lane on 25 Apr 2010 16:30 > I am down to seeing three suspicious rcu_dereference_check traces when > I apply this patch and all the previous patches to 2.6.34-rc5-git6. > > 1. The "__sched_setscheduler+0x19d/0x300" trace. > 2. The two "is_swiotlb_buffer+0x2e/0x3b" traces (waiting to see > Johannes' patch show up in a Linux snapshot) > > Did I miss a patch for the setscheduler issue? Hmm. I am still seeing these two messages as well. [ 83.363146] [ INFO: suspicious rcu_dereference_check() usage. ] [ 83.363148] --------------------------------------------------- [ 83.363151] include/net/inet_timewait_sock.h:227 invoked rcu_dereference_check() without protection! [ 83.363154] [ 83.363155] other info that might help us debug this: [ 83.363156] [ 83.363158] [ 83.363159] rcu_scheduler_active = 1, debug_locks = 1 [ 83.363162] 2 locks held by gwibber-service/5076: [ 83.363164] #0: (&p->lock){+.+.+.}, at: [<ffffffff8110534a>] seq_read+0x37/0x381 [ 83.363176] #1: (&(&hashinfo->ehash_locks[i])->rlock){+.-...}, at: [<ffffffff813ddcd5>] established_get_next+0xc4/0x132 [ 83.363186] [ 83.363187] stack backtrace: [ 83.363191] Pid: 5076, comm: gwibber-service Not tainted 2.6.34-rc5-git6 #27 [ 83.363194] Call Trace: [ 83.363202] [<ffffffff81068086>] lockdep_rcu_dereference+0x9d/0xa5 [ 83.363207] [<ffffffff813dc998>] twsk_net+0x4f/0x57 [ 83.363212] [<ffffffff813ddc65>] established_get_next+0x54/0x132 [ 83.363216] [<ffffffff813dde47>] tcp_seq_next+0x5d/0x6a [ 83.363221] [<ffffffff81105599>] seq_read+0x286/0x381 [ 83.363226] [<ffffffff81105313>] ? seq_read+0x0/0x381 [ 83.363231] [<ffffffff8113503c>] proc_reg_read+0x8d/0xac [ 83.363236] [<ffffffff810ebf14>] vfs_read+0xa6/0x103 [ 83.363241] [<ffffffff810ec027>] sys_read+0x45/0x69 [ 83.363246] [<ffffffff81002b6b>] system_call_fastpath+0x16/0x1b [ 84.660302] [ INFO: suspicious rcu_dereference_check() usage. ] [ 84.660304] --------------------------------------------------- [ 84.660308] include/net/inet_timewait_sock.h:227 invoked rcu_dereference_check() without protection! [ 84.660311] [ 84.660312] other info that might help us debug this: [ 84.660313] [ 84.660315] [ 84.660316] rcu_scheduler_active = 1, debug_locks = 1 [ 84.660319] no locks held by gwibber-service/5081. [ 84.660321] [ 84.660322] stack backtrace: [ 84.660325] Pid: 5081, comm: gwibber-service Not tainted 2.6.34-rc5-git6 #27 [ 84.660328] Call Trace: [ 84.660339] [<ffffffff81068086>] lockdep_rcu_dereference+0x9d/0xa5 [ 84.660345] [<ffffffff813cad6f>] twsk_net+0x4f/0x57 [ 84.660350] [<ffffffff813cb18f>] __inet_twsk_hashdance+0x50/0x158 [ 84.660355] [<ffffffff813e0bb9>] tcp_time_wait+0x1c1/0x24b [ 84.660360] [<ffffffff813d3d97>] tcp_fin+0x83/0x162 [ 84.660364] [<ffffffff813d4727>] tcp_data_queue+0x1ff/0xa1e [ 84.660370] [<ffffffff810496aa>] ? mod_timer+0x1e/0x20 [ 84.660375] [<ffffffff813d8363>] tcp_rcv_state_process+0x89d/0x8f2 [ 84.660381] [<ffffffff813943bb>] ? release_sock+0x30/0x10b [ 84.660386] [<ffffffff813de772>] tcp_v4_do_rcv+0x2de/0x33f [ 84.660391] [<ffffffff8139440d>] release_sock+0x82/0x10b [ 84.660395] [<ffffffff813ce875>] tcp_close+0x1b5/0x37e [ 84.660401] [<ffffffff813ecdb7>] inet_release+0x50/0x57 [ 84.660405] [<ffffffff81391ae4>] sock_release+0x1a/0x66 [ 84.660410] [<ffffffff81391b52>] sock_close+0x22/0x26 [ 84.660415] [<ffffffff810ece07>] __fput+0x120/0x1cd [ 84.660420] [<ffffffff810ecec9>] fput+0x15/0x17 [ 84.660424] [<ffffffff810e9d41>] filp_close+0x63/0x6d [ 84.660428] [<ffffffff810e9e22>] sys_close+0xd7/0x111 [ 84.660434] [<ffffffff81002b6b>] system_call_fastpath+0x16/0x1b -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Miles Lane on 27 Apr 2010 14:00 On Tue, Apr 27, 2010 at 12:22 PM, Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote: > On Mon, Apr 26, 2010 at 09:27:44PM -0700, Paul E. McKenney wrote: >> On Mon, Apr 26, 2010 at 11:35:10AM -0700, Eric W. Biederman wrote: >> > "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com> writes: >> > >> > > Eric Dumazet traced these down to a commit from Eric Biederman. >> > > >> > > If I don't hear from Eric Biederman in a few days, I will attempt a >> > > patch, but it would be more likely to be correct coming from someone >> > > with a better understanding of the code. �;-) >> > >> > I already replied. >> > >> > http://lkml.org/lkml/2010/4/21/420 >> >> You did indeed!!! �This experience is giving me an even better appreciation >> of the maintainers' ability to keep all their patches straight! >> >> I will put together something based on your suggestion. > > How about the following? > > � � � � � � � � � � � � � � � � � � � � � � � � � � � �Thanx, Paul > > ------------------------------------------------------------------------ > > commit 85fa42bd568ab99c375f018761ae6345249942cd > Author: Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> > Date: � Mon Apr 26 21:40:05 2010 -0700 > > � �net: suppress RCU lockdep false positive in twsk_net() > > � �Calls to twsk_net() are in some cases protected by reference counting > � �as an alternative to RCU protection. �Cases covered by reference counts > � �include __inet_twsk_kill(), inet_twsk_free(), inet_twdr_do_twkill_work(), > � �inet_twdr_twcal_tick(), and tcp_timewait_state_process(). �RCU is used > � �by inet_twsk_purge(). �Locking is used by established_get_first() > � �and established_get_next(). �Finally, __inet_twsk_hashdance() is an > � �initialization case. > > � �It appears to be non-trivial to locate the appropriate locks and > � �reference counts from within twsk_net(), so used rcu_dereference_raw(). > > � �Signed-off-by: Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> > > diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h > index 79f67ea..a066fdd 100644 > --- a/include/net/inet_timewait_sock.h > +++ b/include/net/inet_timewait_sock.h > @@ -224,7 +224,9 @@ static inline > �struct net *twsk_net(const struct inet_timewait_sock *twsk) > �{ > �#ifdef CONFIG_NET_NS > - � � � return rcu_dereference(twsk->tw_net); > + � � � return rcu_dereference_raw(twsk->tw_net); /* protected by locking, */ > + � � � � � � � � � � � � � � � � � � � � � � � � /* reference counting, */ > + � � � � � � � � � � � � � � � � � � � � � � � � /* initialization, or RCU. */ > �#else > � � � �return &init_net; > �#endif > Worked for me. Thanks! Miles -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Miles Lane on 1 May 2010 13:40 On Tue, Apr 20, 2010 at 9:52 AM, Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote: > On Tue, Apr 20, 2010 at 08:45:28AM -0400, Miles Lane wrote: >> Is there a patch set for 2.6.34-rc5 I can test? > > I will be sending a patchset out later today after testing, but > please see below for a sneak preview collapsed into a single patch. > > � � � � � � � � � � � � � � � � � � � � � � � � � � � �Thanx, Paul > >> On Tue, Apr 20, 2010 at 8:31 AM, Eric Paris <eparis(a)redhat.com> wrote: >> >> > On Tue, 2010-04-20 at 16:23 +0800, Lai Jiangshan wrote: >> > >> > > [PATCH] RCU: don't turn off lockdep when find suspicious >> > rcu_dereference_check() usage >> > > >> > > When suspicious rcu_dereference_check() usage is detected, lockdep is >> > still >> > > available actually, so we should not call debug_locks_off() in >> > > lockdep_rcu_dereference(). >> > > >> > > For get rid of too much "suspicious rcu_dereference_check() usage" >> > > output when the "if(!debug_locks_off())" statement is removed. This patch >> > uses >> > > static variable '__warned's for very usage of "rcu_dereference*()". >> > > >> > > One variable per usage, so, Now, we can get multiple complaint >> > > when we detect multiple different suspicious rcu_dereference_check() >> > usage. >> > > >> > > Requested-by: Eric Paris <eparis(a)redhat.com> >> > > Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com> >> > >> > Although mine was a linux-next kernel and it doesn't appear that I have >> > rcu_dereference_protected() at all, so I dropped that bit of the patch, >> > it worked great! �I got 4 more complaints to harass people with. �Feel >> > free to add my tested by if you care to. >> > >> > Tested-by: Eric Paris <eparis(a)redhat.com> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 07db2fe..ec9ab49 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -190,6 +190,15 @@ static inline int rcu_read_lock_sched_held(void) > > �#ifdef CONFIG_PROVE_RCU > > +#define __do_rcu_dereference_check(c) � � � � � � � � � � � � � � � � �\ > + � � � do { � � � � � � � � � � � � � � � � � � � � � � � � � � � � � �\ > + � � � � � � � static bool __warned; � � � � � � � � � � � � � � � � � \ > + � � � � � � � if (debug_lockdep_rcu_enabled() && !__warned && !(c)) { \ > + � � � � � � � � � � � __warned = true; � � � � � � � � � � � � � � � �\ > + � � � � � � � � � � � lockdep_rcu_dereference(__FILE__, __LINE__); � �\ > + � � � � � � � } � � � � � � � � � � � � � � � � � � � � � � � � � � � \ > + � � � } while (0) > + > �/** > �* rcu_dereference_check - rcu_dereference with debug checking > �* @p: The pointer to read, prior to dereferencing > @@ -219,8 +228,7 @@ static inline int rcu_read_lock_sched_held(void) > �*/ > �#define rcu_dereference_check(p, c) \ > � � � �({ \ > - � � � � � � � if (debug_lockdep_rcu_enabled() && !(c)) \ > - � � � � � � � � � � � lockdep_rcu_dereference(__FILE__, __LINE__); \ > + � � � � � � � __do_rcu_dereference_check(c); \ > � � � � � � � �rcu_dereference_raw(p); \ > � � � �}) > > @@ -237,8 +245,7 @@ static inline int rcu_read_lock_sched_held(void) > �*/ > �#define rcu_dereference_protected(p, c) \ > � � � �({ \ > - � � � � � � � if (debug_lockdep_rcu_enabled() && !(c)) \ > - � � � � � � � � � � � lockdep_rcu_dereference(__FILE__, __LINE__); \ > + � � � � � � � __do_rcu_dereference_check(c); \ > � � � � � � � �(p); \ > � � � �}) > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c > index da5e139..e5c0244 100644 > --- a/kernel/cgroup_freezer.c > +++ b/kernel/cgroup_freezer.c > @@ -205,9 +205,12 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task) > � � � � * No lock is needed, since the task isn't on tasklist yet, > � � � � * so it can't be moved to another cgroup, which means the > � � � � * freezer won't be removed and will be valid during this > - � � � �* function call. > + � � � �* function call. �Nevertheless, apply RCU read-side critical > + � � � �* section to suppress RCU lockdep false positives. > � � � � */ > + � � � rcu_read_lock(); > � � � �freezer = task_freezer(task); > + � � � rcu_read_unlock(); > > � � � �/* > � � � � * The root cgroup is non-freezable, so we can skip the > diff --git a/kernel/lockdep.c b/kernel/lockdep.c > index 2594e1c..03dd1fa 100644 > --- a/kernel/lockdep.c > +++ b/kernel/lockdep.c > @@ -3801,8 +3801,6 @@ void lockdep_rcu_dereference(const char *file, const int line) > �{ > � � � �struct task_struct *curr = current; > > - � � � if (!debug_locks_off()) > - � � � � � � � return; > � � � �printk("\n===================================================\n"); > � � � �printk( �"[ INFO: suspicious rcu_dereference_check() usage. ]\n"); > � � � �printk( �"---------------------------------------------------\n"); > diff --git a/kernel/sched.c b/kernel/sched.c > index 6af210a..14c44ec 100644 > --- a/kernel/sched.c > +++ b/kernel/sched.c > @@ -323,6 +323,15 @@ static inline struct task_group *task_group(struct task_struct *p) > �/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */ > �static inline void set_task_rq(struct task_struct *p, unsigned int cpu) > �{ > + � � � /* > + � � � �* Strictly speaking this rcu_read_lock() is not needed since the > + � � � �* task_group is tied to the cgroup, which in turn can never go away > + � � � �* as long as there are tasks attached to it. > + � � � �* > + � � � �* However since task_group() uses task_subsys_state() which is an > + � � � �* rcu_dereference() user, this quiets CONFIG_PROVE_RCU. > + � � � �*/ > + � � � rcu_read_lock(); > �#ifdef CONFIG_FAIR_GROUP_SCHED > � � � �p->se.cfs_rq = task_group(p)->cfs_rq[cpu]; > � � � �p->se.parent = task_group(p)->se[cpu]; > @@ -332,6 +341,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu) > � � � �p->rt.rt_rq �= task_group(p)->rt_rq[cpu]; > � � � �p->rt.parent = task_group(p)->rt_se[cpu]; > �#endif > + � � � rcu_read_unlock(); > �} > > �#else > Hi Paul. Has this patch made it into the Linus tree? Thanks! Miles -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Miles Lane on 1 May 2010 22:10
On Sat, May 1, 2010 at 5:55 PM, Paul E. McKenney <paulmck(a)linux.vnet.ibm.com> wrote: > On Sat, May 01, 2010 at 01:26:15PM -0400, Miles Lane wrote: >> On Tue, Apr 20, 2010 at 9:52 AM, Paul E. McKenney >> <paulmck(a)linux.vnet.ibm.com> wrote: >> > On Tue, Apr 20, 2010 at 08:45:28AM -0400, Miles Lane wrote: >> >> Is there a patch set for 2.6.34-rc5 I can test? >> > >> > I will be sending a patchset out later today after testing, but >> > please see below for a sneak preview collapsed into a single patch. >> > >> > � � � � � � � � � � � � � � � � � � � � � � � � � � � �Thanx, Paul >> > >> >> On Tue, Apr 20, 2010 at 8:31 AM, Eric Paris <eparis(a)redhat.com> wrote: >> >> >> >> > On Tue, 2010-04-20 at 16:23 +0800, Lai Jiangshan wrote: >> >> > >> >> > > [PATCH] RCU: don't turn off lockdep when find suspicious >> >> > rcu_dereference_check() usage >> >> > > >> >> > > When suspicious rcu_dereference_check() usage is detected, lockdep is >> >> > still >> >> > > available actually, so we should not call debug_locks_off() in >> >> > > lockdep_rcu_dereference(). >> >> > > >> >> > > For get rid of too much "suspicious rcu_dereference_check() usage" >> >> > > output when the "if(!debug_locks_off())" statement is removed. This patch >> >> > uses >> >> > > static variable '__warned's for very usage of "rcu_dereference*()". >> >> > > >> >> > > One variable per usage, so, Now, we can get multiple complaint >> >> > > when we detect multiple different suspicious rcu_dereference_check() >> >> > usage. >> >> > > >> >> > > Requested-by: Eric Paris <eparis(a)redhat.com> >> >> > > Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com> >> >> > >> >> > Although mine was a linux-next kernel and it doesn't appear that I have >> >> > rcu_dereference_protected() at all, so I dropped that bit of the patch, >> >> > it worked great! �I got 4 more complaints to harass people with. �Feel >> >> > free to add my tested by if you care to. >> >> > >> >> > Tested-by: Eric Paris <eparis(a)redhat.com> >> > >> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h >> > index 07db2fe..ec9ab49 100644 >> > --- a/include/linux/rcupdate.h >> > +++ b/include/linux/rcupdate.h >> > @@ -190,6 +190,15 @@ static inline int rcu_read_lock_sched_held(void) >> > >> > �#ifdef CONFIG_PROVE_RCU >> > >> > +#define __do_rcu_dereference_check(c) � � � � � � � � � � � � � � � � �\ >> > + � � � do { � � � � � � � � � � � � � � � � � � � � � � � � � � � � � �\ >> > + � � � � � � � static bool __warned; � � � � � � � � � � � � � � � � � \ >> > + � � � � � � � if (debug_lockdep_rcu_enabled() && !__warned && !(c)) { \ >> > + � � � � � � � � � � � __warned = true; � � � � � � � � � � � � � � � �\ >> > + � � � � � � � � � � � lockdep_rcu_dereference(__FILE__, __LINE__); � �\ >> > + � � � � � � � } � � � � � � � � � � � � � � � � � � � � � � � � � � � \ >> > + � � � } while (0) >> > + >> > �/** >> > �* rcu_dereference_check - rcu_dereference with debug checking >> > �* @p: The pointer to read, prior to dereferencing >> > @@ -219,8 +228,7 @@ static inline int rcu_read_lock_sched_held(void) >> > �*/ >> > �#define rcu_dereference_check(p, c) \ >> > � � � �({ \ >> > - � � � � � � � if (debug_lockdep_rcu_enabled() && !(c)) \ >> > - � � � � � � � � � � � lockdep_rcu_dereference(__FILE__, __LINE__); \ >> > + � � � � � � � __do_rcu_dereference_check(c); \ >> > � � � � � � � �rcu_dereference_raw(p); \ >> > � � � �}) >> > >> > @@ -237,8 +245,7 @@ static inline int rcu_read_lock_sched_held(void) >> > �*/ >> > �#define rcu_dereference_protected(p, c) \ >> > � � � �({ \ >> > - � � � � � � � if (debug_lockdep_rcu_enabled() && !(c)) \ >> > - � � � � � � � � � � � lockdep_rcu_dereference(__FILE__, __LINE__); \ >> > + � � � � � � � __do_rcu_dereference_check(c); \ >> > � � � � � � � �(p); \ >> > � � � �}) >> > >> > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c >> > index da5e139..e5c0244 100644 >> > --- a/kernel/cgroup_freezer.c >> > +++ b/kernel/cgroup_freezer.c >> > @@ -205,9 +205,12 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task) >> > � � � � * No lock is needed, since the task isn't on tasklist yet, >> > � � � � * so it can't be moved to another cgroup, which means the >> > � � � � * freezer won't be removed and will be valid during this >> > - � � � �* function call. >> > + � � � �* function call. �Nevertheless, apply RCU read-side critical >> > + � � � �* section to suppress RCU lockdep false positives. >> > � � � � */ >> > + � � � rcu_read_lock(); >> > � � � �freezer = task_freezer(task); >> > + � � � rcu_read_unlock(); >> > >> > � � � �/* >> > � � � � * The root cgroup is non-freezable, so we can skip the >> > diff --git a/kernel/lockdep.c b/kernel/lockdep.c >> > index 2594e1c..03dd1fa 100644 >> > --- a/kernel/lockdep.c >> > +++ b/kernel/lockdep.c >> > @@ -3801,8 +3801,6 @@ void lockdep_rcu_dereference(const char *file, const int line) >> > �{ >> > � � � �struct task_struct *curr = current; >> > >> > - � � � if (!debug_locks_off()) >> > - � � � � � � � return; >> > � � � �printk("\n===================================================\n"); >> > � � � �printk( �"[ INFO: suspicious rcu_dereference_check() usage. ]\n"); >> > � � � �printk( �"---------------------------------------------------\n"); >> > diff --git a/kernel/sched.c b/kernel/sched.c >> > index 6af210a..14c44ec 100644 >> > --- a/kernel/sched.c >> > +++ b/kernel/sched.c >> > @@ -323,6 +323,15 @@ static inline struct task_group *task_group(struct task_struct *p) >> > �/* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */ >> > �static inline void set_task_rq(struct task_struct *p, unsigned int cpu) >> > �{ >> > + � � � /* >> > + � � � �* Strictly speaking this rcu_read_lock() is not needed since the >> > + � � � �* task_group is tied to the cgroup, which in turn can never go away >> > + � � � �* as long as there are tasks attached to it. >> > + � � � �* >> > + � � � �* However since task_group() uses task_subsys_state() which is an >> > + � � � �* rcu_dereference() user, this quiets CONFIG_PROVE_RCU. >> > + � � � �*/ >> > + � � � rcu_read_lock(); >> > �#ifdef CONFIG_FAIR_GROUP_SCHED >> > � � � �p->se.cfs_rq = task_group(p)->cfs_rq[cpu]; >> > � � � �p->se.parent = task_group(p)->se[cpu]; >> > @@ -332,6 +341,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu) >> > � � � �p->rt.rt_rq �= task_group(p)->rt_rq[cpu]; >> > � � � �p->rt.parent = task_group(p)->rt_se[cpu]; >> > �#endif >> > + � � � rcu_read_unlock(); >> > �} >> > >> > �#else >> > >> >> Hi Paul. >> >> Has this patch made it into the Linus tree? >> Thanks! > > Hello, Miles, > > Not yet -- working with Ingo to get a variant of it into -tip on > its way to Linus's tree. �The latest patch stack may be found at > http://lkml.org/lkml/2010/4/30/500. > > � � � � � � � � � � � � � � � � � � � � � � � �Thanx, Paul > What is the rationale for defaulting to showing only one RCU splat? That setting seems likely to reduce the rate at which things get cleaned up. Ciao, Miles -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |