br_forward.c - rcu dereference warning

PATCH In review Johannes Berg 4 participants 11 messages Jul 27, 2010 – Jul 28, 2010

Subsystems (3)

Networking Network drivers Ethernet bridge

lore

Share & tools

lore

Johannes Berg

author

2010-07-27 10:45 UTC #0 lore raw

I couldn't find this reported yet, apologies if I missed it.

johannes

[   60.140433] ===================================================
[   60.140437] [ INFO: suspicious rcu_dereference_check() usage. ]
[   60.140440] ---------------------------------------------------
[   60.140444] /home/johannes/sys/wireless-testing/net/bridge/br_forward.c:215 invoked rcu_dereference_check() without protection!
[   60.140447] 
[   60.140448] other info that might help us debug this:
[   60.140449] 
[   60.140452] 
[   60.140453] rcu_scheduler_active = 1, debug_locks = 1
[   60.140457] 2 locks held by Xorg/3083:
[   60.140459]  #0:  (&im->timer){+.-...}, at: [<ffffffff81058180>] call_timer_fn+0x0/0x2f0
[   60.140473]  #1:  (rcu_read_lock_bh){.+....}, at: [<ffffffff813b6c3a>] dev_queue_xmit+0x5a/0x690
[   60.140484] 
[   60.140484] stack backtrace:
[   60.140489] Pid: 3083, comm: Xorg Not tainted 2.6.35-rc6-wl-47665-gc2e2180-dirty #174
[   60.140492] Call Trace:
[   60.140495]  <IRQ>  [<ffffffff8107e494>] lockdep_rcu_dereference+0xa4/0xc0
[   60.140514]  [<ffffffffa07617c3>] br_multicast_flood+0x293/0x310 [bridge]
[   60.140531]  [<ffffffffa0761877>] br_multicast_deliver+0x17/0x20 [bridge]
[   60.140539]  [<ffffffffa07607bc>] br_dev_xmit+0x10c/0x170 [bridge]
[   60.140550]  [<ffffffff813b69ba>] dev_hard_start_xmit+0x21a/0x2e0
[   60.140556]  [<ffffffff813b708e>] dev_queue_xmit+0x4ae/0x690
[   60.140576]  [<ffffffff813c0d43>] neigh_resolve_output+0x113/0x250
[   60.140582]  [<ffffffff813eec46>] ip_finish_output+0x2a6/0x570
[   60.140588]  [<ffffffff813ef33c>] ip_mc_output+0x1dc/0x320
[   60.140593]  [<ffffffff813ed7ed>] ip_local_out+0x2d/0x80
[   60.140600]  [<ffffffff81422236>] igmp_send_report+0x1c6/0x200
[   60.140610]  [<ffffffff814238f0>] igmp_timer_expire+0x100/0x130
[   60.140615]  [<ffffffff81058219>] call_timer_fn+0x99/0x2f0
[   60.140636]  [<ffffffff810585e3>] run_timer_softirq+0x173/0x330
[   60.140641]  [<ffffffff8104f114>] __do_softirq+0x114/0x3d0
[   60.140652]  [<ffffffff8100360c>] call_softirq+0x1c/0x50

Paul E. McKenney

2010-07-27 14:05 UTC #1 lore raw

Quoted 1 line · Johannes Berg

I couldn't find this reported yet, apologies if I missed it.

My first guess is that the call to br_multicast_deliver() in br_dev_xmit() needs to be enclosed in rcu_read_lock(), but I have to defer to someone who knows the code better. Another possible fix would be to change the rcu_dereference() in br_multicast_flood() to rcu_dereference_bh(), for example.

Thanx, Paul

Quoted 40 lines

johannes

[   60.140433] ===================================================
[   60.140437] [ INFO: suspicious rcu_dereference_check() usage. ]
[   60.140440] ---------------------------------------------------
[   60.140444] /home/johannes/sys/wireless-testing/net/bridge/br_forward.c:215 invoked rcu_dereference_check() without protection!
[   60.140447] 
[   60.140448] other info that might help us debug this:
[   60.140449] 
[   60.140452] 
[   60.140453] rcu_scheduler_active = 1, debug_locks = 1
[   60.140457] 2 locks held by Xorg/3083:
[   60.140459]  #0:  (&im->timer){+.-...}, at: [<ffffffff81058180>] call_timer_fn+0x0/0x2f0
[   60.140473]  #1:  (rcu_read_lock_bh){.+....}, at: [<ffffffff813b6c3a>] dev_queue_xmit+0x5a/0x690
[   60.140484] 
[   60.140484] stack backtrace:
[   60.140489] Pid: 3083, comm: Xorg Not tainted 2.6.35-rc6-wl-47665-gc2e2180-dirty #174
[   60.140492] Call Trace:
[   60.140495]  <IRQ>  [<ffffffff8107e494>] lockdep_rcu_dereference+0xa4/0xc0
[   60.140514]  [<ffffffffa07617c3>] br_multicast_flood+0x293/0x310 [bridge]
[   60.140531]  [<ffffffffa0761877>] br_multicast_deliver+0x17/0x20 [bridge]
[   60.140539]  [<ffffffffa07607bc>] br_dev_xmit+0x10c/0x170 [bridge]
[   60.140550]  [<ffffffff813b69ba>] dev_hard_start_xmit+0x21a/0x2e0
[   60.140556]  [<ffffffff813b708e>] dev_queue_xmit+0x4ae/0x690
[   60.140576]  [<ffffffff813c0d43>] neigh_resolve_output+0x113/0x250
[   60.140582]  [<ffffffff813eec46>] ip_finish_output+0x2a6/0x570
[   60.140588]  [<ffffffff813ef33c>] ip_mc_output+0x1dc/0x320
[   60.140593]  [<ffffffff813ed7ed>] ip_local_out+0x2d/0x80
[   60.140600]  [<ffffffff81422236>] igmp_send_report+0x1c6/0x200
[   60.140610]  [<ffffffff814238f0>] igmp_timer_expire+0x100/0x130
[   60.140615]  [<ffffffff81058219>] call_timer_fn+0x99/0x2f0
[   60.140636]  [<ffffffff810585e3>] run_timer_softirq+0x173/0x330
[   60.140641]  [<ffffffff8104f114>] __do_softirq+0x114/0x3d0
[   60.140652]  [<ffffffff8100360c>] call_softirq+0x1c/0x50

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephen Hemminger

2010-07-27 18:26 UTC #2 lore raw

I think this is needed (untested)...
-----
Subject: bridge: add rcu_read_lock on transmit

Long ago, when bridge was converted to RCU, rcu lock was equivalent to having preempt disabled. RCU has changed a lot since then and bridge code was still assuming the since transmit was called with bottom half disabled, it was RCU safe.

Signed-off-by Stephen Hemminger <[email protected]>

---
 net/bridge/br_device.c   |    4 +++-
 net/bridge/br_fdb.c      |    2 +-
 net/bridge/br_input.c    |    7 +++----
 net/bridge/br_stp_bpdu.c |    2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

net/bridge/br_device.c +3 -1

@@ -22,7 +22,7 @@
 #include <asm/uaccess.h>
 #include "br_private.h"
 
-/* net device transmit always called with no BH (preempt_disabled) */
+/* net device transmit always called with no BH disabled */
 netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct net_bridge *br = netdev_priv(dev);

@@ -48,6 +48,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 	skb_reset_mac_header(skb);
 	skb_pull(skb, ETH_HLEN);
 
+	rcu_read_lock();
 	if (is_multicast_ether_addr(dest)) {
 		if (unlikely(netpoll_tx_running(dev))) {
 			br_flood_deliver(br, skb);

@@ -67,6 +68,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 		br_flood_deliver(br, skb);
 
 out:
+	rcu_read_unlock();
 	return NETDEV_TX_OK;
 }

net/bridge/br_fdb.c +1 -1

@@ -214,7 +214,7 @@ void br_fdb_delete_by_port(struct net_br
 	spin_unlock_bh(&br->hash_lock);
 }
 
-/* No locking or refcounting, assumes caller has no preempt (rcu_read_lock) */
+/* No locking or refcounting, assumes caller has rcu_read_lock */
 struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
 					  const unsigned char *addr)
 {

net/bridge/br_input.c +3 -4

@@ -39,7 +39,7 @@ static int br_pass_frame_up(struct sk_bu
 		       netif_receive_skb);
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {
 	const unsigned char *dest = eth_hdr(skb)->h_dest;

@@ -110,7 +110,7 @@ drop:
 	goto out;
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 static int br_handle_local_finish(struct sk_buff *skb)
 {
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);

@@ -133,8 +133,7 @@ static inline int is_link_local(const un
 
 /*
  * Return NULL if skb is handled
- * note: already called with rcu_read_lock (preempt_disabled) from
- * netif_receive_skb
+ * note: already called with rcu_read_lock from netif_receive_skb
  */
 struct sk_buff *br_handle_frame(struct sk_buff *skb)
 {

net/bridge/br_stp_bpdu.c +1 -1

@@ -131,7 +131,7 @@ void br_send_tcn_bpdu(struct net_bridge_
 /*
  * Called from llc.
  *
- * NO locks, but rcu_read_lock (preempt_disabled)
+ * NO locks, but rcu_read_lock
  */
 void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb,
 		struct net_device *dev)

Johannes Berg

author

2010-07-27 20:14 UTC #3 lore raw

net/bridge/br_device.c line 25

Quoted 2 lines · Stephen Hemminger

net/bridge/br_device.c +1 -1

-/* net device transmit always called with no BH (preempt_disabled) */
+/* net device transmit always called with no BH disabled */

"no BH disabled"?

Quoted 61 lines

net/bridge/br_device.c +2 -0

@@ -48,6 +48,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 	skb_reset_mac_header(skb);
 	skb_pull(skb, ETH_HLEN);
 
+	rcu_read_lock();
 	if (is_multicast_ether_addr(dest)) {
 		if (unlikely(netpoll_tx_running(dev))) {
 			br_flood_deliver(br, skb);

@@ -67,6 +68,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 		br_flood_deliver(br, skb);
 
 out:
+	rcu_read_unlock();
 	return NETDEV_TX_OK;
 }

net/bridge/br_fdb.c +1 -1

@@ -214,7 +214,7 @@ void br_fdb_delete_by_port(struct net_br
 	spin_unlock_bh(&br->hash_lock);
 }
 
-/* No locking or refcounting, assumes caller has no preempt (rcu_read_lock) */
+/* No locking or refcounting, assumes caller has rcu_read_lock */
 struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
 					  const unsigned char *addr)
 {

net/bridge/br_input.c +3 -4

@@ -39,7 +39,7 @@ static int br_pass_frame_up(struct sk_bu
 		       netif_receive_skb);
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {
 	const unsigned char *dest = eth_hdr(skb)->h_dest;

@@ -110,7 +110,7 @@ drop:
 	goto out;
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 static int br_handle_local_finish(struct sk_buff *skb)
 {
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);

@@ -133,8 +133,7 @@ static inline int is_link_local(const un
 
 /*
  * Return NULL if skb is handled
- * note: already called with rcu_read_lock (preempt_disabled) from
- * netif_receive_skb
+ * note: already called with rcu_read_lock from netif_receive_skb
  */
 struct sk_buff *br_handle_frame(struct sk_buff *skb)
 {

net/bridge/br_stp_bpdu.c void br_send_tcn_bpdu(struct net_bridge_ · line 137

Quoted 8 lines

net/bridge/br_stp_bpdu.c +1 -1

@@ -131,7 +131,7 @@ void br_send_tcn_bpdu(struct net_bridge_
 /*
  * Called from llc.
  *
- * NO locks, but rcu_read_lock (preempt_disabled)
+ * NO locks, but rcu_read_lock
  */
 void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb,
 		struct net_device *dev)

Did you want me to test the patch?

johannes

Stephen Hemminger

2010-07-27 20:42 UTC #4 lore raw

Quoted 70 lines · On Tue, 27 Jul 2010 22:14:39 +0200 Johannes Berg <[email protected]>…

net/bridge/br_device.c line 25

Quoted 2 lines · Stephen Hemminger

net/bridge/br_device.c +1 -1

-/* net device transmit always called with no BH (preempt_disabled) */
+/* net device transmit always called with no BH disabled */

"no BH disabled"?

Quoted 63 lines

net/bridge/br_device.c +2 -0

@@ -48,6 +48,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 	skb_reset_mac_header(skb);
 	skb_pull(skb, ETH_HLEN);
 
+	rcu_read_lock();
 	if (is_multicast_ether_addr(dest)) {
 		if (unlikely(netpoll_tx_running(dev))) {
 			br_flood_deliver(br, skb);

@@ -67,6 +68,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 		br_flood_deliver(br, skb);
 
 out:
+	rcu_read_unlock();
 	return NETDEV_TX_OK;
 }

net/bridge/br_fdb.c +1 -1

@@ -214,7 +214,7 @@ void br_fdb_delete_by_port(struct net_br
 	spin_unlock_bh(&br->hash_lock);
 }
 
-/* No locking or refcounting, assumes caller has no preempt (rcu_read_lock) */
+/* No locking or refcounting, assumes caller has rcu_read_lock */
 struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
 					  const unsigned char *addr)
 {

net/bridge/br_input.c +3 -4

@@ -39,7 +39,7 @@ static int br_pass_frame_up(struct sk_bu
 		       netif_receive_skb);
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {
 	const unsigned char *dest = eth_hdr(skb)->h_dest;

@@ -110,7 +110,7 @@ drop:
 	goto out;
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 static int br_handle_local_finish(struct sk_buff *skb)
 {
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);

@@ -133,8 +133,7 @@ static inline int is_link_local(const un
 
 /*
  * Return NULL if skb is handled
- * note: already called with rcu_read_lock (preempt_disabled) from
- * netif_receive_skb
+ * note: already called with rcu_read_lock from netif_receive_skb
  */
 struct sk_buff *br_handle_frame(struct sk_buff *skb)
 {

net/bridge/br_stp_bpdu.c +0 -0

@@ -131,7 +131,7 @@ void br_send_tcn_bpdu(struct net_bridge_
 /*
  * Called from llc.
  *

Quoted 8 lines

net/bridge/br_stp_bpdu.c void br_send_tcn_bpdu(struct net_bridge_ · line 137
Quoted 6 lines
net/bridge/br_stp_bpdu.c +1 -1
- * NO locks, but rcu_read_lock (preempt_disabled)
+ * NO locks, but rcu_read_lock
  */
 void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb,
 		struct net_device *dev)
Did you want me to test the patch?

Yes please, I can make sure it works, but not that it gets rid of your error

Johannes Berg

author

2010-07-28 06:48 UTC #5 lore raw

Quoted 4 lines · Stephen Hemminger

Quoted 1 line
Did you want me to test the patch?
Yes please, I can make sure it works, but not that it gets rid of your error

Testing now, but it didn't quite apply cleanly on 2.6.35-rc.

johannes

Johannes Berg

author

2010-07-28 07:33 UTC #6 lore raw

In reply to Stephen Hemminger

Quoted 5 lines · Stephen Hemminger

Quoted 2 lines
Did you want me to test the patch?
Yes please, I can make sure it works, but not that it gets rid of your error

Yes, it fixed it, thanks.

johannes

Stephen Hemminger

2010-07-28 16:57 UTC #7 lore raw

PATCH 1/1 bridge: add rcu_read_lock

In addition to fixing the code, update the comments about locking to match current state as well.

Signed-off-by Stephen Hemminger <[email protected]>

---
 net/bridge/br_device.c   |    4 +++-
 net/bridge/br_fdb.c      |    2 +-
 net/bridge/br_input.c    |    7 +++----
 net/bridge/br_stp_bpdu.c |    2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

net/bridge/br_device.c +3 -1

@@ -22,7 +22,7 @@
 #include <asm/uaccess.h>
 #include "br_private.h"
 
-/* net device transmit always called with no BH (preempt_disabled) */
+/* net device transmit always called with BH disabled */
 netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct net_bridge *br = netdev_priv(dev);

@@ -48,6 +48,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 	skb_reset_mac_header(skb);
 	skb_pull(skb, ETH_HLEN);
 
+	rcu_read_lock();
 	if (is_multicast_ether_addr(dest)) {
 		if (unlikely(netpoll_tx_running(dev))) {
 			br_flood_deliver(br, skb);

@@ -67,6 +68,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
 		br_flood_deliver(br, skb);
 
 out:
+	rcu_read_unlock();
 	return NETDEV_TX_OK;
 }

net/bridge/br_fdb.c +1 -1

@@ -214,7 +214,7 @@ void br_fdb_delete_by_port(struct net_br
 	spin_unlock_bh(&br->hash_lock);
 }
 
-/* No locking or refcounting, assumes caller has no preempt (rcu_read_lock) */
+/* No locking or refcounting, assumes caller has rcu_read_lock */
 struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
 					  const unsigned char *addr)
 {

net/bridge/br_input.c +3 -4

@@ -39,7 +39,7 @@ static int br_pass_frame_up(struct sk_bu
 		       netif_receive_skb);
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {
 	const unsigned char *dest = eth_hdr(skb)->h_dest;

@@ -110,7 +110,7 @@ drop:
 	goto out;
 }
 
-/* note: already called with rcu_read_lock (preempt_disabled) */
+/* note: already called with rcu_read_lock */
 static int br_handle_local_finish(struct sk_buff *skb)
 {
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);

@@ -133,8 +133,7 @@ static inline int is_link_local(const un
 
 /*
  * Return NULL if skb is handled
- * note: already called with rcu_read_lock (preempt_disabled) from
- * netif_receive_skb
+ * note: already called with rcu_read_lock from netif_receive_skb
  */
 struct sk_buff *br_handle_frame(struct sk_buff *skb)
 {

net/bridge/br_stp_bpdu.c +1 -1

@@ -131,7 +131,7 @@ void br_send_tcn_bpdu(struct net_bridge_
 /*
  * Called from llc.
  *
- * NO locks, but rcu_read_lock (preempt_disabled)
+ * NO locks, but rcu_read_lock
  */
 void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb,
 		struct net_device *dev)

David Miller

maintainer

2010-07-28 17:51 UTC #8 lore raw

Re: [PATCH] bridge: add rcu_read_lock

From: Stephen Hemminger <[email protected]>
Date: Wed, 28 Jul 2010 09:57:30 -0700

Quoted 9 lines

Long ago, when bridge was converted to RCU, rcu lock was equivalent to having preempt disabled. RCU has changed a lot since then and bridge code was still assuming the since transmit was called with bottom half disabled, it was RCU safe.
In addition to fixing the code, update the comments about locking to match current state as well.
Signed-off-by Stephen Hemminger <[email protected]>

As I stated in another email I added the original commit.

This version still didn't apply to net-2.6 and also you failed to add a tested-by tag for Johannes to the commit message, which I also did alongside backporting the original patch to net-2.6

Stephen Hemminger

2010-07-28 18:02 UTC #9 lore raw

Re: [PATCH] bridge: add rcu_read_lock

Quoted 11 lines · On Wed, 28 Jul 2010 10:52:09 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 28 Jul 2010 09:57:30 -0700
Quoted 8 lines
Long ago, when bridge was converted to RCU, rcu lock was equivalent to having preempt disabled. RCU has changed a lot since then and bridge code was still assuming the since transmit was called with bottom half disabled, it was RCU safe.
In addition to fixing the code, update the comments about locking to match current state as well.

Quoted 7 lines

Quoted 1 line
Signed-off-by Stephen Hemminger <[email protected]>
As I stated in another email I added the original commit.
This version still didn't apply to net-2.6 and also you failed to add a tested-by tag for Johannes to the commit message, which I also did alongside backporting the original patch to net-2.6

I didn't see yours, if you look at the date it was ships in the night.

David Miller

maintainer

2010-07-28 17:39 UTC #10 lore raw

In reply to Johannes Berg

From: Johannes Berg <[email protected]>
Date: Wed, 28 Jul 2010 09:33:51 +0200

Quoted 9 lines · Stephen Hemminger

Quoted 6 lines
Quoted 3 lines
Did you want me to test the patch?
Yes please, I can make sure it works, but not that it gets rid of your error
Yes, it fixed it, thanks.

I fixed up the comment thinko Johannes noticed, added a tested-by tag, and applied this to net-2.6

Thanks!