Possible race with br_del_if()

Possible race with br_del_if()

[Ryan Harper]

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 <ryanh@us.ibm.com>
---
Simple workaround for double call to br_del_if().

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

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

Re: Possible race with br_del_if()

[Stephen Hemminger]

On Thu, 18 Aug 2005 16:40:36 -0500
Ryan Harper <ryanh@us.ibm.com> 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
> 

Re: Possible race with br_del_if()

[Ryan Harper]

* Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:11]:
> On Thu, 18 Aug 2005 16:40:36 -0500
> Ryan Harper <ryanh@us.ibm.com> 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


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

Re: Possible race with br_del_if()

[Stephen Hemminger]

On Thu, 18 Aug 2005 17:23:23 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> * Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:11]:
> > On Thu, 18 Aug 2005 16:40:36 -0500
> > Ryan Harper <ryanh@us.ibm.com> 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

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

			

Re: Possible race with br_del_if()

[Ryan Harper]

* Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:36]:
> On Thu, 18 Aug 2005 17:23:23 -0500
> Ryan Harper <ryanh@us.ibm.com> wrote:
> 
> > * Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:11]:
> > > On Thu, 18 Aug 2005 16:40:36 -0500
> > > Ryan Harper <ryanh@us.ibm.com> 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
> 
> 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

Re: Possible race with br_del_if()

[Ryan Harper]

* Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:36]:
> On Thu, 18 Aug 2005 17:23:23 -0500
> Ryan Harper <ryanh@us.ibm.com> wrote:
> 
> > * Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:11]:
> > > On Thu, 18 Aug 2005 16:40:36 -0500
> > > Ryan Harper <ryanh@us.ibm.com> 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
> 
> 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

Re: Possible race with br_del_if()

[Stephen Hemminger]

On Fri, 19 Aug 2005 14:10:52 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> * Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:36]:
> > On Thu, 18 Aug 2005 17:23:23 -0500
> > Ryan Harper <ryanh@us.ibm.com> wrote:
> > 
> > > * Stephen Hemminger <shemminger@osdl.org> [2005-08-18 17:11]:
> > > > On Thu, 18 Aug 2005 16:40:36 -0500
> > > > Ryan Harper <ryanh@us.ibm.com> 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
> > 
> > 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.

[PATCH] br: fix race on bridge del if

[Stephen Hemminger]

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 <shemminger@osdl.org>

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

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

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
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 <shemminger@osdl.org>

Applied, thanks Stephen.