[BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Michael Guntsche]

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 

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Herbert Xu]

Michael Guntsche <mike@it-loops.com> wrote:
>
> 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 <mike@it-loops.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index c188c80..32b8f9f 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -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;
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index f3ac1e8..f06ee39 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[David Miller]

From: Herbert Xu <herbert@gondor.hengli.com.au>
Date: Wed, 6 Jul 2011 07:58:33 +0800

> 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 <mike@it-loops.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied.

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Stephen Hemminger]

On Tue, 05 Jul 2011 18:40:44 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Herbert Xu <herbert@gondor.hengli.com.au>
> Date: Wed, 6 Jul 2011 07:58:33 +0800
> 
> > 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 <mike@it-loops.com>
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Applied.

Obviously needs to go to stable as well.

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Herbert Xu]

On Tue, Jul 05, 2011 at 10:06:36PM -0700, Stephen Hemminger wrote:
> On Tue, 05 Jul 2011 18:40:44 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Herbert Xu <herbert@gondor.hengli.com.au>
> > Date: Wed, 6 Jul 2011 07:58:33 +0800
> > 
> > > 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 <mike@it-loops.com>
> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > 
> > 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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Stephen Hemminger]

On Wed, 6 Jul 2011 13:08:48 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Tue, Jul 05, 2011 at 10:06:36PM -0700, Stephen Hemminger wrote:
> > On Tue, 05 Jul 2011 18:40:44 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> > > From: Herbert Xu <herbert@gondor.hengli.com.au>
> > > Date: Wed, 6 Jul 2011 07:58:33 +0800
> > > 
> > > > 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 <mike@it-loops.com>
> > > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > > 
> > > 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.

--- a/net/bridge/br_input.c	2011-07-05 22:28:30.111995701 -0700
+++ b/net/bridge/br_input.c	2011-07-05 22:34:08.259995671 -0700
@@ -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) ||

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Herbert Xu]

On Tue, Jul 05, 2011 at 10:35:51PM -0700, Stephen Hemminger wrote:
>
> 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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUG] bd4265fe36 bridge: Only flood unreg groups... breaks DHCP setup

[Michael Guntsche]

On 06 Jul 11 07:58, Herbert Xu wrote:
> Michael Guntsche <mike@it-loops.com> wrote:
> >
> > 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