[PATCH net-next 0/4] Bridge IGMP cleanup and fixes

[PATCH net-next 0/4] Bridge IGMP cleanup and fixes

[Stephen Hemminger]

-- 

[PATCH net-next 1/4] bridge: simplify multicast_add_router

[Stephen Hemminger]

[-- Attachment #1: br-mcast-list2.patch --]
[-- Type: text/plain, Size: 1172 bytes --]

By coding slightly differently, there are only two cases
to deal with: add at head and add after previous entry.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/bridge/br_multicast.c	2010-04-27 17:02:06.720839467 -0700
+++ b/net/bridge/br_multicast.c	2010-04-27 17:11:06.518933878 -0700
@@ -1039,22 +1039,25 @@ static int br_ip6_multicast_mld2_report(
 }
 #endif
 
+/*
+ * Add port to rotuer_list
+ *  list is maintained ordered by pointer value
+ *  and locked by br->multicast_lock and RCU
+ */
 static void br_multicast_add_router(struct net_bridge *br,
 				    struct net_bridge_port *port)
 {
 	struct net_bridge_port *p;
-	struct hlist_node *n, *last = NULL;
+	struct hlist_node *n, *slot = NULL;
 
 	hlist_for_each_entry(p, n, &br->router_list, rlist) {
-		if ((unsigned long) port >= (unsigned long) p) {
-			hlist_add_before_rcu(n, &port->rlist);
-			return;
-		}
-		last = n;
+		if ((unsigned long) port >= (unsigned long) p)
+			break;
+		slot = n;
 	}
 
-	if (last)
-		hlist_add_after_rcu(last, &port->rlist);
+	if (slot)
+		hlist_add_after_rcu(slot, &port->rlist);
 	else
 		hlist_add_head_rcu(&port->rlist, &br->router_list);
 }

-- 

[PATCH net-next 2/4] bridge: multicast flood

[Stephen Hemminger]

[-- Attachment #1: br-router-list-rcu.patch --]
[-- Type: text/plain, Size: 786 bytes --]

Fix unsafe usage of RCU. Would never work on Alpha SMP because
of lack of rcu_dereference()

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/bridge/br_forward.c	2010-04-27 17:11:52.008626080 -0700
+++ b/net/bridge/br_forward.c	2010-04-27 17:23:46.388602995 -0700
@@ -216,7 +216,7 @@ static void br_multicast_flood(struct ne
 
 	prev = NULL;
 
-	rp = br->router_list.first;
+	rp = rcu_dereference(br->router_list.first);
 	p = mdst ? mdst->ports : NULL;
 	while (p || rp) {
 		lport = p ? p->port : NULL;
@@ -233,7 +233,7 @@ static void br_multicast_flood(struct ne
 		if ((unsigned long)lport >= (unsigned long)port)
 			p = p->next;
 		if ((unsigned long)rport >= (unsigned long)port)
-			rp = rp->next;
+			rp = rcu_dereference(rp->next);
 	}
 
 	if (!prev)

-- 

[PATCH net-next 3/4] bridge: multicast port group RCU fix

[Stephen Hemminger]

[-- Attachment #1: br-portlist-rcu.patch --]
[-- Type: text/plain, Size: 1463 bytes --]

The recently introduced bridge mulitcast port group list was only
partially using RCU correctly. It was missing rcu_dereference()
and missing the necessary barrier on deletion.

The code should have used one of the standard list methods (list or hlist)
instead of open coding a RCU based link list.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/bridge/br_forward.c	2010-04-27 17:51:27.909588950 -0700
+++ b/net/bridge/br_forward.c	2010-04-27 17:53:18.790721091 -0700
@@ -217,7 +217,7 @@ static void br_multicast_flood(struct ne
 	prev = NULL;
 
 	rp = rcu_dereference(br->router_list.first);
-	p = mdst ? mdst->ports : NULL;
+	p = mdst ? rcu_dereference(mdst->ports) : NULL;
 	while (p || rp) {
 		lport = p ? p->port : NULL;
 		rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
@@ -231,7 +231,7 @@ static void br_multicast_flood(struct ne
 			goto out;
 
 		if ((unsigned long)lport >= (unsigned long)port)
-			p = p->next;
+			p = rcu_dereference(p->next);
 		if ((unsigned long)rport >= (unsigned long)port)
 			rp = rcu_dereference(rp->next);
 	}
--- a/net/bridge/br_multicast.c	2010-04-27 17:51:31.509593914 -0700
+++ b/net/bridge/br_multicast.c	2010-04-27 17:52:48.209243982 -0700
@@ -259,7 +259,7 @@ static void br_multicast_del_pg(struct n
 		if (p != pg)
 			continue;
 
-		*pp = p->next;
+		rcu_assign_pointer(*pp, p->next);
 		hlist_del_init(&p->mglist);
 		del_timer(&p->timer);
 		del_timer(&p->query_timer);

-- 

[PATCH net-next 4/4] bridge: multicast_flood cleanup

[Stephen Hemminger]

[-- Attachment #1: br-flood-clean.patch --]
[-- Type: text/plain, Size: 958 bytes --]

Move some declarations around to make it clearer which variables
are being used inside loop.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/bridge/br_forward.c	2010-04-27 17:58:25.739592056 -0700
+++ b/net/bridge/br_forward.c	2010-04-27 17:59:17.182654034 -0700
@@ -208,17 +208,15 @@ static void br_multicast_flood(struct ne
 {
 	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
 	struct net_bridge *br = netdev_priv(dev);
-	struct net_bridge_port *port;
-	struct net_bridge_port *lport, *rport;
-	struct net_bridge_port *prev;
+	struct net_bridge_port *prev = NULL;
 	struct net_bridge_port_group *p;
 	struct hlist_node *rp;
 
-	prev = NULL;
-
 	rp = rcu_dereference(br->router_list.first);
 	p = mdst ? rcu_dereference(mdst->ports) : NULL;
 	while (p || rp) {
+		struct net_bridge_port *port, *lport, *rport;
+
 		lport = p ? p->port : NULL;
 		rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
 			     NULL;

-- 

Re: [PATCH net-next 0/4] Bridge IGMP cleanup and fixes

[David Miller]

All looks good, I'll apply to net-next-2.6 and build test.

Thanks Stephen.

Re: [PATCH net-next 3/4] bridge: multicast port group RCU fix

[Herbert Xu]

On Tue, Apr 27, 2010 at 06:01:06PM -0700, Stephen Hemminger wrote:
> The recently introduced bridge mulitcast port group list was only
> partially using RCU correctly. It was missing rcu_dereference()
> and missing the necessary barrier on deletion.
> 
> The code should have used one of the standard list methods (list or hlist)
> instead of open coding a RCU based link list.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/net/bridge/br_forward.c	2010-04-27 17:51:27.909588950 -0700
> +++ b/net/bridge/br_forward.c	2010-04-27 17:53:18.790721091 -0700
> @@ -217,7 +217,7 @@ static void br_multicast_flood(struct ne
>  	prev = NULL;
>  
>  	rp = rcu_dereference(br->router_list.first);
> -	p = mdst ? mdst->ports : NULL;
> +	p = mdst ? rcu_dereference(mdst->ports) : NULL;
>  	while (p || rp) {
>  		lport = p ? p->port : NULL;
>  		rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
> @@ -231,7 +231,7 @@ static void br_multicast_flood(struct ne
>  			goto out;
>  
>  		if ((unsigned long)lport >= (unsigned long)port)
> -			p = p->next;
> +			p = rcu_dereference(p->next);
>  		if ((unsigned long)rport >= (unsigned long)port)
>  			rp = rcu_dereference(rp->next);
>  	}

Thanks for catching this!

> --- a/net/bridge/br_multicast.c	2010-04-27 17:51:31.509593914 -0700
> +++ b/net/bridge/br_multicast.c	2010-04-27 17:52:48.209243982 -0700
> @@ -259,7 +259,7 @@ static void br_multicast_del_pg(struct n
>  		if (p != pg)
>  			continue;
>  
> -		*pp = p->next;
> +		rcu_assign_pointer(*pp, p->next);

But this is bogus.  br_multicast_del_pg is removing an entry from
the RCU list.

You only need write barriers when you're putting a new entry in
it, and only if there is no other barrier between the code filling
in the new entry and the line adding it to the RCU list.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next 3/4] bridge: multicast port group RCU fix

[Stephen Hemminger]

On Wed, 28 Apr 2010 11:07:09 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Tue, Apr 27, 2010 at 06:01:06PM -0700, Stephen Hemminger wrote:
> > The recently introduced bridge mulitcast port group list was only
> > partially using RCU correctly. It was missing rcu_dereference()
> > and missing the necessary barrier on deletion.
> > 
> > The code should have used one of the standard list methods (list or hlist)
> > instead of open coding a RCU based link list.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > --- a/net/bridge/br_forward.c	2010-04-27 17:51:27.909588950 -0700
> > +++ b/net/bridge/br_forward.c	2010-04-27 17:53:18.790721091 -0700
> > @@ -217,7 +217,7 @@ static void br_multicast_flood(struct ne
> >  	prev = NULL;
> >  
> >  	rp = rcu_dereference(br->router_list.first);
> > -	p = mdst ? mdst->ports : NULL;
> > +	p = mdst ? rcu_dereference(mdst->ports) : NULL;
> >  	while (p || rp) {
> >  		lport = p ? p->port : NULL;
> >  		rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
> > @@ -231,7 +231,7 @@ static void br_multicast_flood(struct ne
> >  			goto out;
> >  
> >  		if ((unsigned long)lport >= (unsigned long)port)
> > -			p = p->next;
> > +			p = rcu_dereference(p->next);
> >  		if ((unsigned long)rport >= (unsigned long)port)
> >  			rp = rcu_dereference(rp->next);
> >  	}
> 
> Thanks for catching this!
> 
> > --- a/net/bridge/br_multicast.c	2010-04-27 17:51:31.509593914 -0700
> > +++ b/net/bridge/br_multicast.c	2010-04-27 17:52:48.209243982 -0700
> > @@ -259,7 +259,7 @@ static void br_multicast_del_pg(struct n
> >  		if (p != pg)
> >  			continue;
> >  
> > -		*pp = p->next;
> > +		rcu_assign_pointer(*pp, p->next);
> 
> But this is bogus.  br_multicast_del_pg is removing an entry from
> the RCU list.
> 
> You only need write barriers when you're putting a new entry in
> it, and only if there is no other barrier between the code filling
> in the new entry and the line adding it to the RCU list.

Yeah, it is extra barrier (one more reason to stick to hlist_del_rcu)




--