Skip to contentSkip to conversation

Bridge IGMP cleanup and fixes

PATCH In review Stephen Hemminger 3 participants 8 messages Apr 28, 2010

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore
author
#1 lore raw

PATCH 1/4 bridge: simplify multicast_add_router

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 <[email protected]>
net/bridge/br_multicast.c +11 -8
@@ -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);
 }
author
#2 lore raw

PATCH 2/4 bridge: multicast flood

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>
net/bridge/br_forward.c +2 -2
@@ -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)
author
#3 lore raw

PATCH 3/4 bridge: multicast port group RCU fix

In reply to Stephen Hemminger

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 <[email protected]>
net/bridge/br_forward.c +2 -2
@@ -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);
 	}
net/bridge/br_multicast.c +1 -1
@@ -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);
#4 lore raw

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

Quoted 21 lines · Stephen Hemminger

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 <[email protected]>
net/bridge/br_forward.c +1 -1
@@ -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) :

net/bridge/br_forward.c static void br_multicast_flood(struct ne · line 237

Quoted 8 lines
net/bridge/br_forward.c +1 -1
@@ -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!

net/bridge/br_multicast.c static void br_multicast_del_pg(struct n · line 262

Quoted 8 lines
net/bridge/br_multicast.c +1 -1
@@ -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,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#5 lore raw

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

Quoted 45 lines · On Wed, 28 Apr 2010 11:07:09 +0800 Herbert Xu <[email protected]>…
Quoted 21 lines · Stephen Hemminger

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 <[email protected]>
net/bridge/br_forward.c +1 -1
@@ -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) :

net/bridge/br_forward.c static void br_multicast_flood(struct ne · line 237

Quoted 8 lines
net/bridge/br_forward.c +1 -1
@@ -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!

net/bridge/br_multicast.c static void br_multicast_del_pg(struct n · line 262

Quoted 8 lines
net/bridge/br_multicast.c +1 -1
@@ -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.

Quoted 3 lines

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)

author
#6 lore raw

PATCH 4/4 bridge: multicast_flood cleanup

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>
net/bridge/br_forward.c +3 -5
@@ -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;