Possible race with br_del_if()

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

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com


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

Signed-off-by: Ryan Harper <redacted>
---
Simple workaround for double call to br_del_if().

Signed-off-by: Ryan Harper <redacted>

--- 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);

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper [off-list ref] 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?

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

* Stephen Hemminger [off-list ref] [2005-08-18 17:11]:

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper [off-list ref] wrote:

quoted

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


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

On Thu, 18 Aug 2005 17:23:23 -0500
Ryan Harper [off-list ref] wrote:

* Stephen Hemminger [off-list ref] [2005-08-18 17:11]:

quoted

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper [off-list ref] wrote:

quoted

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

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

			

* Stephen Hemminger [off-list ref] [2005-08-18 17:36]:

On Thu, 18 Aug 2005 17:23:23 -0500
Ryan Harper [off-list ref] wrote:

quoted

* Stephen Hemminger [off-list ref] [2005-08-18 17:11]:

quoted

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper [off-list ref] wrote:

quoted

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

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?

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

* Stephen Hemminger [off-list ref] [2005-08-18 17:36]:

On Thu, 18 Aug 2005 17:23:23 -0500
Ryan Harper [off-list ref] wrote:

quoted

* Stephen Hemminger [off-list ref] [2005-08-18 17:11]:

quoted

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper [off-list ref] wrote:

quoted

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

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().  

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

On Fri, 19 Aug 2005 14:10:52 -0500
Ryan Harper [off-list ref] wrote:

* Stephen Hemminger [off-list ref] [2005-08-18 17:36]:

quoted

On Thu, 18 Aug 2005 17:23:23 -0500
Ryan Harper [off-list ref] wrote:

quoted

* Stephen Hemminger [off-list ref] [2005-08-18 17:11]:

quoted

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper [off-list ref] wrote:

quoted

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

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().  

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

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 <redacted>

Index: nexgate-test/net/bridge/br_if.c
===================================================================

--- nexgate-test.orig/net/bridge/br_if.c
+++ 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);

From: Stephen Hemminger <redacted>
Date: Tue, 11 Oct 2005 13:33:28 -0700

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 <redacted>

Applied, thanks Stephen.