Skip to contentSkip to conversation

Possible race with br_del_if()

PATCH Applied Ryan Harper 3 participants 9 messages Aug 18, 2005 – Oct 12, 2005

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore

Ryan Harper

author
#0 lore raw

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

CPU0                    CPU1
add_del_if()            unregister_netdevice()  
br_del_if()             notifier_call_chain(NETDEV_UNREGISTER) 
del_nbp()               
br_stp_disable_port()   // port->state == BR_STATE_DISABLED
                        br_device_event() // dev->br_port != NULL yet
                                          // event is NETDEV_UNREGISTER
                        br_del_if()
                        sysfs_remove_dir(p)
                        kobject_del()
                        dget(dentry)
                        BUG_ON(!atomic_read(&dentry->d_count)

This sequence doesn't happen all of the time. In many cases, CPU0 moves along right into destroy_nbp() which sets dev->br_port = NULL, and be_device_event check (p == NULL) hits and a second br_del_if() isn't called.

The attached patch is a workaround for the double case, but I'm not sure if is the right way to deal with this issue, or if it any issue at all.

1. http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=90

Signature · 28 lines
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
[email protected]


diffstat output:
 br_if.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Signed-off-by: Ryan Harper <[email protected]>
---
Simple workaround for double call to br_del_if().

Signed-off-by: Ryan Harper <[email protected]>

--- linux-2.6.12/net/bridge/br_if.c	2005-06-17 14:48:29.000000000 -0500
+++ linux-2.6.12-xen0-smp/net/bridge/br_if.c	2005-08-18 15:17:27.302615846 -0500
@@ -382,7 +382,7 @@
 {
 	struct net_bridge_port *p = dev->br_port;
 	
-	if (!p || p->br != br) 
+	if (!p || p->br != br || p->state == BR_STATE_DISABLED)
 		return -EINVAL;
 
 	br_sysfs_removeif(p);
#1 lore raw
Quoted 5 lines · On Thu, 18 Aug 2005 16:40:36 -0500 Ryan Harper <[email protected]> wrote:

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

Doesn't the mutex in RTNL work right? or are you calling routines with out asserting it?

Quoted 23 lines
CPU0                    CPU1
add_del_if()            unregister_netdevice()  
br_del_if()             notifier_call_chain(NETDEV_UNREGISTER) 
del_nbp()               
br_stp_disable_port()   // port->state == BR_STATE_DISABLED
                        br_device_event() // dev->br_port != NULL yet
                                          // event is NETDEV_UNREGISTER
                        br_del_if()
                        sysfs_remove_dir(p)
                        kobject_del()
                        dget(dentry)
                        BUG_ON(!atomic_read(&dentry->d_count)

This sequence doesn't happen all of the time. In many cases, CPU0 moves along right into destroy_nbp() which sets dev->br_port = NULL, and be_device_event check (p == NULL) hits and a second br_del_if() isn't called.

The attached patch is a workaround for the double case, but I'm not sure if is the right way to deal with this issue, or if it any issue at all.

1. http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=90

author
#2 lore raw
Quoted 11 lines · Stephen Hemminger
Quoted 5 lines · On Thu, 18 Aug 2005 16:40:36 -0500 Ryan Harper <[email protected]> wrote:

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

Doesn't the mutex in RTNL work right? or are you calling routines with out asserting it?

unregister_netdevice asserts RTNL, add_del_if() in br_ioctl.c doesn't seem to do so. I don't see it down dev_get_by_index() path either. It looks like any caller of add_del_if() isn't asserting RTNL. The two callers I see are:

br_dev_ioctl() in br_ioctl.c
old_dev_ioctl() in br_ioctl.c

Signature · 5 lines
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
[email protected]
#3 lore raw
Quoted 13 lines · On Thu, 18 Aug 2005 17:23:23 -0500 Ryan Harper <[email protected]> wrote:
Quoted 11 lines · Stephen Hemminger
Quoted 5 lines · On Thu, 18 Aug 2005 16:40:36 -0500 Ryan Harper <[email protected]> wrote:

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

Doesn't the mutex in RTNL work right? or are you calling routines with out asserting it?

Quoted 7 lines

unregister_netdevice asserts RTNL, add_del_if() in br_ioctl.c doesn't seem to do so. I don't see it down dev_get_by_index() path either. It looks like any caller of add_del_if() isn't asserting RTNL. The two callers I see are:

br_dev_ioctl() in br_ioctl.c
old_dev_ioctl() in br_ioctl.c

But the pat to br_dev_ioctl() is via the socket ioctl and that should already have gotten RTNL.

dev_ioctl

	rtnl_lock()
	dev_ifsioc()
		dev->do_ioctl --> br_dev_ioctl
author
#4 lore raw
Quoted 24 lines · Stephen Hemminger
Quoted 13 lines · On Thu, 18 Aug 2005 17:23:23 -0500 Ryan Harper <[email protected]> wrote:
Quoted 11 lines · Stephen Hemminger
Quoted 5 lines · On Thu, 18 Aug 2005 16:40:36 -0500 Ryan Harper <[email protected]> wrote:

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

Doesn't the mutex in RTNL work right? or are you calling routines with out asserting it?

Quoted 7 lines

unregister_netdevice asserts RTNL, add_del_if() in br_ioctl.c doesn't seem to do so. I don't see it down dev_get_by_index() path either. It looks like any caller of add_del_if() isn't asserting RTNL. The two callers I see are:

br_dev_ioctl() in br_ioctl.c
old_dev_ioctl() in br_ioctl.c

Quoted 8 lines

But the pat to br_dev_ioctl() is via the socket ioctl and that should already have gotten RTNL.

dev_ioctl

	rtnl_lock()
	dev_ifsioc()
		dev->do_ioctl --> br_dev_ioctl

Hrm. OK. It sounds like both paths are doing the right thing w.r.t asserting RTNL, but br_device_event() still gets called with:

1) dev->br_port != NULL
2) dev->br_port->state = BR_STATE_DISABLED
3) event = NETDEV_UNREGISTER

which results in br_del_if() being called a second time on the same port.

Some of the other cases (NETDEV_FEAT_CHANGE, NETDEV_CHANGE) do a state check before calling a subsequent function. Does it make sense for br_del_if() to be called on a port whose state is BR_STATE_DISABLED?

Signature · 5 lines
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
[email protected]
author
#5 lore raw
In reply to Stephen Hemminger
Quoted 24 lines · Stephen Hemminger
Quoted 13 lines · On Thu, 18 Aug 2005 17:23:23 -0500 Ryan Harper <[email protected]> wrote:
Quoted 11 lines · Stephen Hemminger
Quoted 5 lines · On Thu, 18 Aug 2005 16:40:36 -0500 Ryan Harper <[email protected]> wrote:

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

Doesn't the mutex in RTNL work right? or are you calling routines with out asserting it?

Quoted 7 lines

unregister_netdevice asserts RTNL, add_del_if() in br_ioctl.c doesn't seem to do so. I don't see it down dev_get_by_index() path either. It looks like any caller of add_del_if() isn't asserting RTNL. The two callers I see are:

br_dev_ioctl() in br_ioctl.c
old_dev_ioctl() in br_ioctl.c

Quoted 8 lines

But the pat to br_dev_ioctl() is via the socket ioctl and that should already have gotten RTNL.

dev_ioctl

	rtnl_lock()
	dev_ifsioc()
		dev->do_ioctl --> br_dev_ioctl

Just to follow-up, the issue was a race between the call_rcu() callback for destroy_nbp() and an unregister_netdev() call. Sometimes the br_device_event() routine was triggered and destroy_nbp() had not been run yet leaving dev->br_port non-NULL to which br_device_event then correctly calls br_del_if().

We caused this by issuing a brctl delif from userspace scripts and having a in kernel handler invoke unregister_netdev() call.

Our fix is to not bother calling brctl delif because the unregister_netdev() call will automatically remove the device from the bridge when the notify_call_chain() kicks in from unregister_netdevice().

Signature · 5 lines
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
[email protected]
#6 lore raw
Quoted 41 lines · On Fri, 19 Aug 2005 14:10:52 -0500 Ryan Harper <[email protected]> wrote:
Quoted 24 lines · Stephen Hemminger
Quoted 13 lines · On Thu, 18 Aug 2005 17:23:23 -0500 Ryan Harper <[email protected]> wrote:
Quoted 11 lines · Stephen Hemminger
Quoted 5 lines · On Thu, 18 Aug 2005 16:40:36 -0500 Ryan Harper <[email protected]> wrote:

Hello,

I've encountered several oops when adding and removing interfaces from bridges while using Xen. Most of the details are available [1]here. The short of it is the following sequence:

Doesn't the mutex in RTNL work right? or are you calling routines with out asserting it?

Quoted 7 lines

unregister_netdevice asserts RTNL, add_del_if() in br_ioctl.c doesn't seem to do so. I don't see it down dev_get_by_index() path either. It looks like any caller of add_del_if() isn't asserting RTNL. The two callers I see are:

br_dev_ioctl() in br_ioctl.c
old_dev_ioctl() in br_ioctl.c

Quoted 8 lines

But the pat to br_dev_ioctl() is via the socket ioctl and that should already have gotten RTNL.

dev_ioctl

	rtnl_lock()
	dev_ifsioc()
		dev->do_ioctl --> br_dev_ioctl

Just to follow-up, the issue was a race between the call_rcu() callback for destroy_nbp() and an unregister_netdev() call. Sometimes the br_device_event() routine was triggered and destroy_nbp() had not been run yet leaving dev->br_port non-NULL to which br_device_event then correctly calls br_del_if().

Quoted 7 lines

We caused this by issuing a brctl delif from userspace scripts and having a in kernel handler invoke unregister_netdev() call.

Our fix is to not bother calling brctl delif because the unregister_netdev() call will automatically remove the device from the bridge when the notify_call_chain() kicks in from unregister_netdevice().

I'll get back to you, this needs some review, I have a bunch of old test suites to dig up for it.

#7 lore raw

PATCH 1/1 br: fix race on bridge del if

In reply to Ryan Harper

This fixes the RCU race on bridge delete interface. Basically, the network device has to be detached from the bridge in the first step (pre-RCU), rather than later. At that point, no more bridge traffic will come in, and the other code will not think that network device is part of a bridge.

This should also fix the XEN test problems. If there is another 2.6.13-stable, add it as well.

Signed-off-by Stephen Hemminger <[email protected]>
net/bridge/br_if.c +1 -1
Index: nexgate-test/net/bridge/br_if.c
===================================================================
@@ -79,7 +79,6 @@ static void destroy_nbp(struct net_bridg
 {
 	struct net_device *dev = p->dev;
 
-	dev->br_port = NULL;
 	p->br = NULL;
 	p->dev = NULL;
 	dev_put(dev);

@@ -100,6 +99,7 @@ static void del_nbp(struct net_bridge_po
 	struct net_bridge *br = p->br;
 	struct net_device *dev = p->dev;
 
+	dev->br_port = NULL;
 	dev_set_promiscuity(dev, -1);
 
 	spin_lock_bh(&br->lock);
maintainer Applied
#8 lore raw

Re: [PATCH] br: fix race on bridge del if

From: Stephen Hemminger <[email protected]>
Date: Tue, 11 Oct 2005 13:33:28 -0700

Quoted 10 lines

This fixes the RCU race on bridge delete interface. Basically, the network device has to be detached from the bridge in the first step (pre-RCU), rather than later. At that point, no more bridge traffic will come in, and the other code will not think that network device is part of a bridge.

This should also fix the XEN test problems. If there is another 2.6.13-stable, add it as well.

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

Applied, thanks Stephen.