Skip to contentSkip to conversation

bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

BUG Applied Michael Guntsche 4 participants 8 messages Jul 5, 2011 – Jul 6, 2011

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

Michael Guntsche

author
#0 lore raw

Hello,

After updating from 3.0.0-rc5 to rc6 I noticed that my cellphone was no longer able to get an address assigned from my DHCP server. While trying to figure out the problem I noticed that tracing with tcpdump made it work again.

The setup I have here is the following:
PPC embedded board where the wired NIC and the wlan NIC are in a bridge. Dnsmasq is listening on the Bridge device itself.

Looking at the changes between rc5 and rc6 I noticed commit

bd4265fe365c0f3945d: bridge: Only flood unregistered groups to routers

For testing purposes I reverted it and the cellphone immediately got an address even without running tcpdump. Now apparently the commit states that the user can always force flooding behaviour to any given port by marking it as a router but I did not find any documentation how to do that.

Now my question? Is it "normal" that this change breaks my setup here and if it is expected how can I force my ports to the old behaviour without reverting the commit.

Thank you very much in advance,
Michael

#1 lore raw
Quoted 4 lines · Michael Guntsche

Looking at the changes between rc5 and rc6 I noticed commit

bd4265fe365c0f3945d: bridge: Only flood unregistered groups to routers

Oops, that was definitely my fault. This patch should fix your problem.

bridge: Always flood broadcast packets

As is_multicast_ether_addr returns true on broadcast packets as well, we need to explicitly exclude broadcast packets so that they're always flooded. This wasn't an issue before as broadcast packets were considered to be an unregistered multicast group, which were always flooded. However, as we now only flood such packets to router ports, this is no longer acceptable.

Reported-by Michael Guntsche <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
net/bridge/br_device.c +3 -1
index c188c80..32b8f9f 100644
@@ -49,7 +49,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb_pull(skb, ETH_HLEN);
 
 	rcu_read_lock();
-	if (is_multicast_ether_addr(dest)) {
+	if (is_broadcast_ether_addr(dest))
+		br_flood_deliver(br, skb);
+	else if (is_multicast_ether_addr(dest)) {
 		if (unlikely(netpoll_tx_running(dev))) {
 			br_flood_deliver(br, skb);
 			goto out;
net/bridge/br_input.c +4 -2
index f3ac1e8..f06ee39 100644
@@ -60,7 +60,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	br = p->br;
 	br_fdb_update(br, p, eth_hdr(skb)->h_source);
 
-	if (is_multicast_ether_addr(dest) &&
+	if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
 	    br_multicast_rcv(br, p, skb))
 		goto drop;
 

@@ -77,7 +77,9 @@ int br_handle_frame_finish(struct sk_buff *skb)
 
 	dst = NULL;
 
-	if (is_multicast_ether_addr(dest)) {
+	if (is_broadcast_ether_addr(dest))
+		skb2 = skb;
+	else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb);
 		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
 			if ((mdst && mdst->mglist) ||

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer Applied
#2 lore raw

From: Herbert Xu <[email protected]>
Date: Wed, 6 Jul 2011 07:58:33 +0800

Quoted 11 lines

bridge: Always flood broadcast packets

As is_multicast_ether_addr returns true on broadcast packets as well, we need to explicitly exclude broadcast packets so that they're always flooded. This wasn't an issue before as broadcast packets were considered to be an unregistered multicast group, which were always flooded. However, as we now only flood such packets to router ports, this is no longer acceptable.

Reported-by Michael Guntsche <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Applied.

#3 lore raw
Quoted 12 lines · On Tue, 05 Jul 2011 18:40:44 -0700 (PDT) David Miller <[email protected]>…

From: Herbert Xu <[email protected]>
Date: Wed, 6 Jul 2011 07:58:33 +0800

Quoted 9 lines

bridge: Always flood broadcast packets

As is_multicast_ether_addr returns true on broadcast packets as well, we need to explicitly exclude broadcast packets so that they're always flooded. This wasn't an issue before as broadcast packets were considered to be an unregistered multicast group, which were always flooded. However, as we now only flood such packets to router ports, this is no longer acceptable.

Quoted 4 lines
Quoted 2 lines
Reported-by Michael Guntsche <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Applied.

Obviously needs to go to stable as well.

#4 lore raw
Quoted 15 lines · Stephen Hemminger
Quoted 12 lines · On Tue, 05 Jul 2011 18:40:44 -0700 (PDT) David Miller <[email protected]>…

From: Herbert Xu <[email protected]>
Date: Wed, 6 Jul 2011 07:58:33 +0800

Quoted 9 lines

bridge: Always flood broadcast packets

As is_multicast_ether_addr returns true on broadcast packets as well, we need to explicitly exclude broadcast packets so that they're always flooded. This wasn't an issue before as broadcast packets were considered to be an unregistered multicast group, which were always flooded. However, as we now only flood such packets to router ports, this is no longer acceptable.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Reported-by Michael Guntsche <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Applied.

Obviously needs to go to stable as well.

I don't think the buggy patch has made it to a release kernel yet.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#5 lore raw
Quoted 19 lines · On Wed, 6 Jul 2011 13:08:48 +0800 Herbert Xu <[email protected]>…
Quoted 18 lines · Stephen Hemminger
Quoted 15 lines · On Tue, 05 Jul 2011 18:40:44 -0700 (PDT) David Miller <[email protected]>…

From: Herbert Xu <[email protected]>
Date: Wed, 6 Jul 2011 07:58:33 +0800

Quoted 11 lines

bridge: Always flood broadcast packets

As is_multicast_ether_addr returns true on broadcast packets as well, we need to explicitly exclude broadcast packets so that they're always flooded. This wasn't an issue before as broadcast packets were considered to be an unregistered multicast group, which were always flooded. However, as we now only flood such packets to router ports, this is no longer acceptable.

Reported-by Michael Guntsche <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Applied.

Obviously needs to go to stable as well.

I don't think the buggy patch has made it to a release kernel yet.

Thanks,

The bisected commit bd4265fe36 is in 3.0-rc4 but the input code path still treats multicast and broadcast the same which means there are some other possible cases where broadcast doesn't get forwarded.

Wouldn't it make more sense to force input path to always forward broadcasts. It would also save a lookup of mdb entry.

net/bridge/br_input.c +3 -1
@@ -77,7 +77,9 @@ int br_handle_frame_finish(struct sk_buf
 
 	dst = NULL;
 
-	if (is_multicast_ether_addr(dest)) {
+	if (is_broadcast_ether_addr(dest))
+		skb2 = skb;
+	else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb);
 		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
 			if ((mdst && mdst->mglist) ||
#6 lore raw
Quoted 5 lines · Stephen Hemminger

The bisected commit bd4265fe36 is in 3.0-rc4 but the input code path still treats multicast and broadcast the same which means there are some other possible cases where broadcast doesn't get forwarded.

AFAIK it worked properly prior to bd4265fe36 because broadcast addresses simply get treated as an unregistered multicast group and ends up being flooded.

So while yes we do do an extra mdb lookup I don't think there is any visible problem in stable.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#7 lore raw
In reply to Herbert Xu
Quoted 8 lines · Herbert Xu
Quoted 4 lines · Michael Guntsche

Looking at the changes between rc5 and rc6 I noticed commit

bd4265fe365c0f3945d: bridge: Only flood unregistered groups to routers

Oops, that was definitely my fault. This patch should fix your problem.

Good morning,

I can confirm that this fixes the issues I am seeing. The Bridge sees the DHCP packets again.

Thank you very much for the quick fix,
Michael