* [PATCH] Fix RCU warning in rt_cache_seq_show
@ 2011-08-09 17:02 Mark Rutland
2011-08-09 17:18 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Mark Rutland @ 2011-08-09 17:02 UTC (permalink / raw)
To: netdev; +Cc: Mark Rutland, David S. Miller, Eric Dumazet, Gergely Kalman
Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()")
added rcu protection to dst neighbour, and updated callsites for
dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show.
This produces a warning on v3.1-rc1 (on a preemptible kernel, on an
ARM Vexpress A9x4):
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
include/net/dst.h:91 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
2 locks held by proc01/32159:
stack backtrace:
[<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4)
[<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] (seq_read+0x324/0x4a4)
[<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] (proc_reg_read+0x70/0x94)
[<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] (vfs_read+0xb0/0x144)
[<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] (sys_read+0x40/0x70)
[<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] (ret_fast_syscall+0x0/0x3c)
This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show,
protecting the dereferenced variable, and clearing the warning.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu>
---
net/ipv4/route.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e3dec1c..6699ef7 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file *seq, void *v)
struct neighbour *n;
int len;
+ rcu_read_lock();
n = dst_get_neighbour(&r->dst);
seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t"
"%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n",
@@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file *seq, void *v)
-1,
(n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0,
r->rt_spec_dst, &len);
+ rcu_read_unlock();
seq_printf(seq, "%*s\n", 127 - len, "");
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] Fix RCU warning in rt_cache_seq_show 2011-08-09 17:02 [PATCH] Fix RCU warning in rt_cache_seq_show Mark Rutland @ 2011-08-09 17:18 ` Eric Dumazet 2011-08-10 1:11 ` Paul E. McKenney [not found] ` <4e424f6c.12cde30a.131e.ffffec9bSMTPIN_ADDED@mx.google.com> 0 siblings, 2 replies; 7+ messages in thread From: Eric Dumazet @ 2011-08-09 17:18 UTC (permalink / raw) To: Mark Rutland, Paul E. McKenney; +Cc: netdev, David S. Miller, Gergely Kalman Le mardi 09 août 2011 à 18:02 +0100, Mark Rutland a écrit : > Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()") > added rcu protection to dst neighbour, and updated callsites for > dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show. > > This produces a warning on v3.1-rc1 (on a preemptible kernel, on an > ARM Vexpress A9x4): > > =================================================== > [ INFO: suspicious rcu_dereference_check() usage. ] > --------------------------------------------------- > include/net/dst.h:91 invoked rcu_dereference_check() without protection! > > other info that might help us debug this: > > rcu_scheduler_active = 1, debug_locks = 0 > 2 locks held by proc01/32159: > > stack backtrace: > [<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) > [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] (seq_read+0x324/0x4a4) > [<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] (proc_reg_read+0x70/0x94) > [<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] (vfs_read+0xb0/0x144) > [<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] (sys_read+0x40/0x70) > [<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] (ret_fast_syscall+0x0/0x3c) > > This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show, > protecting the dereferenced variable, and clearing the warning. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: David S. Miller <davem@davemloft.net> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu> > --- > net/ipv4/route.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index e3dec1c..6699ef7 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file *seq, void *v) > struct neighbour *n; > int len; > > + rcu_read_lock(); > n = dst_get_neighbour(&r->dst); > seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t" > "%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n", > @@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file *seq, void *v) > -1, > (n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0, > r->rt_spec_dst, &len); > + rcu_read_unlock(); > > seq_printf(seq, "%*s\n", 127 - len, ""); > } Hmm, I though rcu_read_lock_bh() (done by caller of this function) was protecting us here. Paul, is it really needed, or is it a lockdep artifact ? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix RCU warning in rt_cache_seq_show 2011-08-09 17:18 ` Eric Dumazet @ 2011-08-10 1:11 ` Paul E. McKenney [not found] ` <4e424f6c.12cde30a.131e.ffffec9bSMTPIN_ADDED@mx.google.com> 1 sibling, 0 replies; 7+ messages in thread From: Paul E. McKenney @ 2011-08-10 1:11 UTC (permalink / raw) To: Eric Dumazet; +Cc: Mark Rutland, netdev, David S. Miller, Gergely Kalman On Tue, Aug 09, 2011 at 07:18:56PM +0200, Eric Dumazet wrote: > Le mardi 09 août 2011 à 18:02 +0100, Mark Rutland a écrit : > > Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()") > > added rcu protection to dst neighbour, and updated callsites for > > dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show. > > > > This produces a warning on v3.1-rc1 (on a preemptible kernel, on an > > ARM Vexpress A9x4): > > > > =================================================== > > [ INFO: suspicious rcu_dereference_check() usage. ] > > --------------------------------------------------- > > include/net/dst.h:91 invoked rcu_dereference_check() without protection! > > > > other info that might help us debug this: > > > > rcu_scheduler_active = 1, debug_locks = 0 > > 2 locks held by proc01/32159: This is very strange. It says that there are two locks held by the task, but refuses to list them. Maybe something stomped on the list of held locks? Thanx, Paul > > stack backtrace: > > [<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) > > [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] (seq_read+0x324/0x4a4) > > [<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] (proc_reg_read+0x70/0x94) > > [<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] (vfs_read+0xb0/0x144) > > [<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] (sys_read+0x40/0x70) > > [<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] (ret_fast_syscall+0x0/0x3c) > > > > This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show, > > protecting the dereferenced variable, and clearing the warning. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: David S. Miller <davem@davemloft.net> > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu> > > --- > > net/ipv4/route.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > > index e3dec1c..6699ef7 100644 > > --- a/net/ipv4/route.c > > +++ b/net/ipv4/route.c > > @@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file *seq, void *v) > > struct neighbour *n; > > int len; > > > > + rcu_read_lock(); > > n = dst_get_neighbour(&r->dst); > > seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t" > > "%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n", > > @@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file *seq, void *v) > > -1, > > (n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0, > > r->rt_spec_dst, &len); > > + rcu_read_unlock(); > > > > seq_printf(seq, "%*s\n", 127 - len, ""); > > } > > > Hmm, I though rcu_read_lock_bh() (done by caller of this function) was > protecting us here. > > > Paul, is it really needed, or is it a lockdep artifact ? > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <4e424f6c.12cde30a.131e.ffffec9bSMTPIN_ADDED@mx.google.com>]
* RE: [PATCH] Fix RCU warning in rt_cache_seq_show [not found] ` <4e424f6c.12cde30a.131e.ffffec9bSMTPIN_ADDED@mx.google.com> @ 2011-08-11 16:58 ` Eric Dumazet 2011-08-12 2:32 ` Paul E. McKenney 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-08-11 16:58 UTC (permalink / raw) To: Mark Rutland, Paul E. McKenney; +Cc: netdev, David S. Miller, Gergely Kalman Le mercredi 10 août 2011 à 10:28 +0100, Mark Rutland a écrit : > > -----Original Message----- > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > Sent: 09 August 2011 18:19 > > To: Mark Rutland; Paul E. McKenney > > Cc: netdev@vger.kernel.org; David S. Miller; Gergely Kalman > > Subject: Re: [PATCH] Fix RCU warning in rt_cache_seq_show > > > > Le mardi 09 août 2011 à 18:02 +0100, Mark Rutland a écrit : > > > Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()") > > > added rcu protection to dst neighbour, and updated callsites for > > > dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show. > > > > > > This produces a warning on v3.1-rc1 (on a preemptible kernel, on an > > > ARM Vexpress A9x4): > > > > > > =================================================== > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > > --------------------------------------------------- > > > include/net/dst.h:91 invoked rcu_dereference_check() without > > protection! > > > > > > other info that might help us debug this: > > > > > > rcu_scheduler_active = 1, debug_locks = 0 > > > 2 locks held by proc01/32159: > > > > > > stack backtrace: > > > [<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] > > (rt_cache_seq_show+0x18c/0x1c4) > > > [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] > > (seq_read+0x324/0x4a4) > > > [<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] > > (proc_reg_read+0x70/0x94) > > > [<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] > > (vfs_read+0xb0/0x144) > > > [<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] > > (sys_read+0x40/0x70) > > > [<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] > > (ret_fast_syscall+0x0/0x3c) > > > > > > This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show, > > > protecting the dereferenced variable, and clearing the warning. > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > Cc: David S. Miller <davem@davemloft.net> > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > > Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu> > > > --- > > > net/ipv4/route.c | 2 ++ > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > > > index e3dec1c..6699ef7 100644 > > > --- a/net/ipv4/route.c > > > +++ b/net/ipv4/route.c > > > @@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file > > *seq, void *v) > > > struct neighbour *n; > > > int len; > > > > > > + rcu_read_lock(); > > > n = dst_get_neighbour(&r->dst); > > > seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t" > > > "%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n", > > > @@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file > > *seq, void *v) > > > -1, > > > (n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0, > > > r->rt_spec_dst, &len); > > > + rcu_read_unlock(); > > > > > > seq_printf(seq, "%*s\n", 127 - len, ""); > > > } > > > > > > Hmm, I though rcu_read_lock_bh() (done by caller of this function) was > > protecting us here. > > Aha. Being a bit trigger-happy, I'd had a quick look at the functions > mentioned in the backtrace, and not looked at any possible inlining. > > This being my first real exposure to RCU, I wasn't aware of the *_bh > variants. Looking at the documentation (Documentation/RCU/checklist.txt), > I think the real problem is that we should be using rcu_dereference_bh in > this case: > > > read-side critical sections are delimited by rcu_read_lock() > > and rcu_read_unlock(), or by similar primitives such as > > rcu_read_lock_bh() and rcu_read_unlock_bh(), in which case > > the matching rcu_dereference() primitive must be used in order > > to keep lockdep happy, in this case, rcu_dereference_bh(). Hmm. I do think dst_get_neighbour() should use rcu_dereference(), because dst->_neighbour are freed by call_rcu(). The question is : Is following construct [A] safe or not ? { rcu_read_lock_bh(); /* BH are now disabled, and we are not allowed to sleep */ ... ptr = rcu_dereference(); ... rcu_read_unlock_bh(); } I dont really understand why lockdep wants [B] instead : { rcu_read_lock_bh(); ... { rcu_read_lock(); ptr = rcu_dereference(); rcu_read_unlock(); } ... rcu_read_unlock_bh(); } However, I can understand the other way [C], this is really needed : { rcu_read_lock(); ... { rcu_read_lock_bh(); ptr = rcu_dereference_bh(); rcu_read_unlock_bh(); } ... rcu_read_unlock(); } I believe [A] should be allowed by lockdep. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix RCU warning in rt_cache_seq_show 2011-08-11 16:58 ` Eric Dumazet @ 2011-08-12 2:32 ` Paul E. McKenney 2011-08-12 7:23 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Paul E. McKenney @ 2011-08-12 2:32 UTC (permalink / raw) To: Eric Dumazet; +Cc: Mark Rutland, netdev, David S. Miller, Gergely Kalman On Thu, Aug 11, 2011 at 06:58:21PM +0200, Eric Dumazet wrote: > Le mercredi 10 août 2011 à 10:28 +0100, Mark Rutland a écrit : > > > -----Original Message----- > > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > > Sent: 09 August 2011 18:19 > > > To: Mark Rutland; Paul E. McKenney > > > Cc: netdev@vger.kernel.org; David S. Miller; Gergely Kalman > > > Subject: Re: [PATCH] Fix RCU warning in rt_cache_seq_show > > > > > > Le mardi 09 août 2011 à 18:02 +0100, Mark Rutland a écrit : > > > > Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()") > > > > added rcu protection to dst neighbour, and updated callsites for > > > > dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show. > > > > > > > > This produces a warning on v3.1-rc1 (on a preemptible kernel, on an > > > > ARM Vexpress A9x4): > > > > > > > > =================================================== > > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > > > --------------------------------------------------- > > > > include/net/dst.h:91 invoked rcu_dereference_check() without > > > protection! > > > > > > > > other info that might help us debug this: > > > > > > > > rcu_scheduler_active = 1, debug_locks = 0 > > > > 2 locks held by proc01/32159: > > > > > > > > stack backtrace: > > > > [<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] > > > (rt_cache_seq_show+0x18c/0x1c4) > > > > [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] > > > (seq_read+0x324/0x4a4) > > > > [<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] > > > (proc_reg_read+0x70/0x94) > > > > [<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] > > > (vfs_read+0xb0/0x144) > > > > [<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] > > > (sys_read+0x40/0x70) > > > > [<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] > > > (ret_fast_syscall+0x0/0x3c) > > > > > > > > This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show, > > > > protecting the dereferenced variable, and clearing the warning. > > > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > Cc: David S. Miller <davem@davemloft.net> > > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > > > Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu> > > > > --- > > > > net/ipv4/route.c | 2 ++ > > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > > > > index e3dec1c..6699ef7 100644 > > > > --- a/net/ipv4/route.c > > > > +++ b/net/ipv4/route.c > > > > @@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file > > > *seq, void *v) > > > > struct neighbour *n; > > > > int len; > > > > > > > > + rcu_read_lock(); > > > > n = dst_get_neighbour(&r->dst); > > > > seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t" > > > > "%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n", > > > > @@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file > > > *seq, void *v) > > > > -1, > > > > (n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0, > > > > r->rt_spec_dst, &len); > > > > + rcu_read_unlock(); > > > > > > > > seq_printf(seq, "%*s\n", 127 - len, ""); > > > > } > > > > > > > > > Hmm, I though rcu_read_lock_bh() (done by caller of this function) was > > > protecting us here. > > > > Aha. Being a bit trigger-happy, I'd had a quick look at the functions > > mentioned in the backtrace, and not looked at any possible inlining. > > > > This being my first real exposure to RCU, I wasn't aware of the *_bh > > variants. Looking at the documentation (Documentation/RCU/checklist.txt), > > I think the real problem is that we should be using rcu_dereference_bh in > > this case: > > > > > read-side critical sections are delimited by rcu_read_lock() > > > and rcu_read_unlock(), or by similar primitives such as > > > rcu_read_lock_bh() and rcu_read_unlock_bh(), in which case > > > the matching rcu_dereference() primitive must be used in order > > > to keep lockdep happy, in this case, rcu_dereference_bh(). > > Hmm. > > I do think dst_get_neighbour() should use rcu_dereference(), because > dst->_neighbour are freed by call_rcu(). > > The question is : Is following construct [A] safe or not ? > > { > rcu_read_lock_bh(); > /* BH are now disabled, and we are not allowed to sleep */ > ... > > ptr = rcu_dereference(); This should be: ptr = rcu_dereference_bh(); As you say below. Never mind! ;-) > ... > rcu_read_unlock_bh(); > } > > > I dont really understand why lockdep wants [B] instead : > > { > rcu_read_lock_bh(); > ... > > { > rcu_read_lock(); > ptr = rcu_dereference(); Here you are protected by both RCU and RCU-bh, so you should be able to use either rcu_dereference() or rcu_dereference_bh(). A bit strange to use rcu_dereference_bh(), though. Except perhaps if a pointer to a function was passed in from the outer RCU-bh read-side critical section or something. > rcu_read_unlock(); > } > ... > rcu_read_unlock_bh(); > } > > > > However, I can understand the other way [C], this is really needed : > > { > rcu_read_lock(); > ... > > { > rcu_read_lock_bh(); > ptr = rcu_dereference_bh(); > rcu_read_unlock_bh(); > } > ... > rcu_read_unlock(); > } > > I believe [A] should be allowed by lockdep. OK, I'll bite. Why? Thanx, Paul ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix RCU warning in rt_cache_seq_show 2011-08-12 2:32 ` Paul E. McKenney @ 2011-08-12 7:23 ` Eric Dumazet 2011-08-12 12:49 ` Paul E. McKenney 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-08-12 7:23 UTC (permalink / raw) To: paulmck; +Cc: Mark Rutland, netdev, David S. Miller, Gergely Kalman Le jeudi 11 août 2011 à 19:32 -0700, Paul E. McKenney a écrit : > On Thu, Aug 11, 2011 at 06:58:21PM +0200, Eric Dumazet wrote: > > Le mercredi 10 août 2011 à 10:28 +0100, Mark Rutland a écrit : > > > > -----Original Message----- > > > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > > > Sent: 09 August 2011 18:19 > > > > To: Mark Rutland; Paul E. McKenney > > > > Cc: netdev@vger.kernel.org; David S. Miller; Gergely Kalman > > > > Subject: Re: [PATCH] Fix RCU warning in rt_cache_seq_show > > > > > > > > Le mardi 09 août 2011 à 18:02 +0100, Mark Rutland a écrit : > > > > > Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()") > > > > > added rcu protection to dst neighbour, and updated callsites for > > > > > dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show. > > > > > > > > > > This produces a warning on v3.1-rc1 (on a preemptible kernel, on an > > > > > ARM Vexpress A9x4): > > > > > > > > > > =================================================== > > > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > > > > --------------------------------------------------- > > > > > include/net/dst.h:91 invoked rcu_dereference_check() without > > > > protection! > > > > > > > > > > other info that might help us debug this: > > > > > > > > > > rcu_scheduler_active = 1, debug_locks = 0 > > > > > 2 locks held by proc01/32159: > > > > > > > > > > stack backtrace: > > > > > [<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] > > > > (rt_cache_seq_show+0x18c/0x1c4) > > > > > [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] > > > > (seq_read+0x324/0x4a4) > > > > > [<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] > > > > (proc_reg_read+0x70/0x94) > > > > > [<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] > > > > (vfs_read+0xb0/0x144) > > > > > [<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] > > > > (sys_read+0x40/0x70) > > > > > [<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] > > > > (ret_fast_syscall+0x0/0x3c) > > > > > > > > > > This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show, > > > > > protecting the dereferenced variable, and clearing the warning. > > > > > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > > Cc: David S. Miller <davem@davemloft.net> > > > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > > > > Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu> > > > > > --- > > > > > net/ipv4/route.c | 2 ++ > > > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > > > > > > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > > > > > index e3dec1c..6699ef7 100644 > > > > > --- a/net/ipv4/route.c > > > > > +++ b/net/ipv4/route.c > > > > > @@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file > > > > *seq, void *v) > > > > > struct neighbour *n; > > > > > int len; > > > > > > > > > > + rcu_read_lock(); > > > > > n = dst_get_neighbour(&r->dst); > > > > > seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t" > > > > > "%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n", > > > > > @@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file > > > > *seq, void *v) > > > > > -1, > > > > > (n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0, > > > > > r->rt_spec_dst, &len); > > > > > + rcu_read_unlock(); > > > > > > > > > > seq_printf(seq, "%*s\n", 127 - len, ""); > > > > > } > > > > > > > > > > > > Hmm, I though rcu_read_lock_bh() (done by caller of this function) was > > > > protecting us here. > > > > > > Aha. Being a bit trigger-happy, I'd had a quick look at the functions > > > mentioned in the backtrace, and not looked at any possible inlining. > > > > > > This being my first real exposure to RCU, I wasn't aware of the *_bh > > > variants. Looking at the documentation (Documentation/RCU/checklist.txt), > > > I think the real problem is that we should be using rcu_dereference_bh in > > > this case: > > > > > > > read-side critical sections are delimited by rcu_read_lock() > > > > and rcu_read_unlock(), or by similar primitives such as > > > > rcu_read_lock_bh() and rcu_read_unlock_bh(), in which case > > > > the matching rcu_dereference() primitive must be used in order > > > > to keep lockdep happy, in this case, rcu_dereference_bh(). > > > > Hmm. > > > > I do think dst_get_neighbour() should use rcu_dereference(), because > > dst->_neighbour are freed by call_rcu(). > > > > The question is : Is following construct [A] safe or not ? > > > > { > > rcu_read_lock_bh(); > > /* BH are now disabled, and we are not allowed to sleep */ > > ... > > > > ptr = rcu_dereference(); > > This should be: > > ptr = rcu_dereference_bh(); > > As you say below. Never mind! ;-) > > > ... > > rcu_read_unlock_bh(); > > } > > > > > > I dont really understand why lockdep wants [B] instead : > > > > { > > rcu_read_lock_bh(); > > ... > > > > { > > rcu_read_lock(); > > ptr = rcu_dereference(); > > Here you are protected by both RCU and RCU-bh, so you should be able > to use either rcu_dereference() or rcu_dereference_bh(). A bit > strange to use rcu_dereference_bh(), though. Except perhaps if a > pointer to a function was passed in from the outer RCU-bh read-side > critical section or something. > > > rcu_read_unlock(); > > } > > ... > > rcu_read_unlock_bh(); > > } > > > > > > > > However, I can understand the other way [C], this is really needed : > > > > { > > rcu_read_lock(); > > ... > > > > { > > rcu_read_lock_bh(); > > ptr = rcu_dereference_bh(); > > rcu_read_unlock_bh(); > > } > > ... > > rcu_read_unlock(); > > } > > > > I believe [A] should be allowed by lockdep. > > OK, I'll bite. Why? > Oh well, I assumed local_bh_disable() disables preemption. It does since day-0 add_preempt_count(SOFTIRQ_DISABLE_OFFSET); So following should be safe : local_bh_disable(); { ptr = rcu_dereference(...); use(ptr); } local_bh_enable(); Maybe they are longterm plans to break this assumption, I dont know. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix RCU warning in rt_cache_seq_show 2011-08-12 7:23 ` Eric Dumazet @ 2011-08-12 12:49 ` Paul E. McKenney 0 siblings, 0 replies; 7+ messages in thread From: Paul E. McKenney @ 2011-08-12 12:49 UTC (permalink / raw) To: Eric Dumazet; +Cc: Mark Rutland, netdev, David S. Miller, Gergely Kalman On Fri, Aug 12, 2011 at 09:23:50AM +0200, Eric Dumazet wrote: > Le jeudi 11 août 2011 à 19:32 -0700, Paul E. McKenney a écrit : > > On Thu, Aug 11, 2011 at 06:58:21PM +0200, Eric Dumazet wrote: > > > Le mercredi 10 août 2011 à 10:28 +0100, Mark Rutland a écrit : > > > > > -----Original Message----- > > > > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > > > > Sent: 09 August 2011 18:19 > > > > > To: Mark Rutland; Paul E. McKenney > > > > > Cc: netdev@vger.kernel.org; David S. Miller; Gergely Kalman > > > > > Subject: Re: [PATCH] Fix RCU warning in rt_cache_seq_show > > > > > > > > > > Le mardi 09 août 2011 à 18:02 +0100, Mark Rutland a écrit : > > > > > > Commit f2c31e32 ("net: fix NULL dereferences in check_peer_redir()") > > > > > > added rcu protection to dst neighbour, and updated callsites for > > > > > > dst_{get,set}_neighbour. Unfortunately, it missed rt_cache_seq_show. > > > > > > > > > > > > This produces a warning on v3.1-rc1 (on a preemptible kernel, on an > > > > > > ARM Vexpress A9x4): > > > > > > > > > > > > =================================================== > > > > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > > > > > --------------------------------------------------- > > > > > > include/net/dst.h:91 invoked rcu_dereference_check() without > > > > > protection! > > > > > > > > > > > > other info that might help us debug this: > > > > > > > > > > > > rcu_scheduler_active = 1, debug_locks = 0 > > > > > > 2 locks held by proc01/32159: > > > > > > > > > > > > stack backtrace: > > > > > > [<80014880>] (unwind_backtrace+0x0/0xf8) from [<802e5c78>] > > > > > (rt_cache_seq_show+0x18c/0x1c4) > > > > > > [<802e5c78>] (rt_cache_seq_show+0x18c/0x1c4) from [<800e0c5c>] > > > > > (seq_read+0x324/0x4a4) > > > > > > [<800e0c5c>] (seq_read+0x324/0x4a4) from [<8010786c>] > > > > > (proc_reg_read+0x70/0x94) > > > > > > [<8010786c>] (proc_reg_read+0x70/0x94) from [<800c0ba8>] > > > > > (vfs_read+0xb0/0x144) > > > > > > [<800c0ba8>] (vfs_read+0xb0/0x144) from [<800c0ea8>] > > > > > (sys_read+0x40/0x70) > > > > > > [<800c0ea8>] (sys_read+0x40/0x70) from [<8000e0c0>] > > > > > (ret_fast_syscall+0x0/0x3c) > > > > > > > > > > > > This patch adds calls to rcu_read_{lock,unlock} in rt_cache_seq_show, > > > > > > protecting the dereferenced variable, and clearing the warning. > > > > > > > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > > > Cc: David S. Miller <davem@davemloft.net> > > > > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > > > > > Cc: Gergely Kalman <synapse@hippy.csoma.elte.hu> > > > > > > --- > > > > > > net/ipv4/route.c | 2 ++ > > > > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > > > > > > > > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > > > > > > index e3dec1c..6699ef7 100644 > > > > > > --- a/net/ipv4/route.c > > > > > > +++ b/net/ipv4/route.c > > > > > > @@ -419,6 +419,7 @@ static int rt_cache_seq_show(struct seq_file > > > > > *seq, void *v) > > > > > > struct neighbour *n; > > > > > > int len; > > > > > > > > > > > > + rcu_read_lock(); > > > > > > n = dst_get_neighbour(&r->dst); > > > > > > seq_printf(seq, "%s\t%08X\t%08X\t%8X\t%d\t%u\t%d\t" > > > > > > "%08X\t%d\t%u\t%u\t%02X\t%d\t%1d\t%08X%n", > > > > > > @@ -435,6 +436,7 @@ static int rt_cache_seq_show(struct seq_file > > > > > *seq, void *v) > > > > > > -1, > > > > > > (n && (n->nud_state & NUD_CONNECTED)) ? 1 : 0, > > > > > > r->rt_spec_dst, &len); > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > seq_printf(seq, "%*s\n", 127 - len, ""); > > > > > > } > > > > > > > > > > > > > > > Hmm, I though rcu_read_lock_bh() (done by caller of this function) was > > > > > protecting us here. > > > > > > > > Aha. Being a bit trigger-happy, I'd had a quick look at the functions > > > > mentioned in the backtrace, and not looked at any possible inlining. > > > > > > > > This being my first real exposure to RCU, I wasn't aware of the *_bh > > > > variants. Looking at the documentation (Documentation/RCU/checklist.txt), > > > > I think the real problem is that we should be using rcu_dereference_bh in > > > > this case: > > > > > > > > > read-side critical sections are delimited by rcu_read_lock() > > > > > and rcu_read_unlock(), or by similar primitives such as > > > > > rcu_read_lock_bh() and rcu_read_unlock_bh(), in which case > > > > > the matching rcu_dereference() primitive must be used in order > > > > > to keep lockdep happy, in this case, rcu_dereference_bh(). > > > > > > Hmm. > > > > > > I do think dst_get_neighbour() should use rcu_dereference(), because > > > dst->_neighbour are freed by call_rcu(). > > > > > > The question is : Is following construct [A] safe or not ? > > > > > > { > > > rcu_read_lock_bh(); > > > /* BH are now disabled, and we are not allowed to sleep */ > > > ... > > > > > > ptr = rcu_dereference(); > > > > This should be: > > > > ptr = rcu_dereference_bh(); > > > > As you say below. Never mind! ;-) > > > > > ... > > > rcu_read_unlock_bh(); > > > } > > > > > > > > > I dont really understand why lockdep wants [B] instead : > > > > > > { > > > rcu_read_lock_bh(); > > > ... > > > > > > { > > > rcu_read_lock(); > > > ptr = rcu_dereference(); > > > > Here you are protected by both RCU and RCU-bh, so you should be able > > to use either rcu_dereference() or rcu_dereference_bh(). A bit > > strange to use rcu_dereference_bh(), though. Except perhaps if a > > pointer to a function was passed in from the outer RCU-bh read-side > > critical section or something. > > > > > rcu_read_unlock(); > > > } > > > ... > > > rcu_read_unlock_bh(); > > > } > > > > > > > > > > > > However, I can understand the other way [C], this is really needed : > > > > > > { > > > rcu_read_lock(); > > > ... > > > > > > { > > > rcu_read_lock_bh(); > > > ptr = rcu_dereference_bh(); > > > rcu_read_unlock_bh(); > > > } > > > ... > > > rcu_read_unlock(); > > > } > > > > > > I believe [A] should be allowed by lockdep. > > > > OK, I'll bite. Why? > > > > Oh well, I assumed local_bh_disable() disables preemption. > > It does since day-0 > add_preempt_count(SOFTIRQ_DISABLE_OFFSET); > > So following should be safe : > > local_bh_disable(); > { > ptr = rcu_dereference(...); > use(ptr); > } > local_bh_enable(); > > Maybe they are longterm plans to break this assumption, I dont know. It would be safe for TINY_RCU and TREE_RCU, but not for either TINY_PREEMPT_RCU or TREE_PREEMPT_RCU. These last two do not recognize a preempt-disable region as an RCU read-side critical section. Thanx, Paul ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-08-12 14:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-09 17:02 [PATCH] Fix RCU warning in rt_cache_seq_show Mark Rutland
2011-08-09 17:18 ` Eric Dumazet
2011-08-10 1:11 ` Paul E. McKenney
[not found] ` <4e424f6c.12cde30a.131e.ffffec9bSMTPIN_ADDED@mx.google.com>
2011-08-11 16:58 ` Eric Dumazet
2011-08-12 2:32 ` Paul E. McKenney
2011-08-12 7:23 ` Eric Dumazet
2011-08-12 12:49 ` Paul E. McKenney
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).