[PATCH] bonding: fix bridged bonds in 802.3ad mode

[PATCH] bonding: fix bridged bonds in 802.3ad mode

[Jiri Bohac]

802.3ad bonding inside a bridge is broken again. Originally fixed by
43aa1920117801fe9ae3d1fad886b62511e09bee, the bug was re-introduced by
1e253c3b8a1aeed51eef6fc366812f219b97de65.

LACP frames must not have their skb->dev changed by the bridging hook.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1514,6 +1514,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
 		memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
 	}
 
+	/* prevent bridging code from mangling and forwarding LACP frames */
+	if (bond->params.mode == BOND_MODE_8023AD &&
+	    skb->protocol == htons(ETH_P_SLOW))
+		return RX_HANDLER_PASS;
+
 	return RX_HANDLER_ANOTHER;
 }
 
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH] bonding: fix bridged bonds in 802.3ad mode

[Ben Hutchings]

On Thu, 2011-04-21 at 20:47 +0200, Jiri Bohac wrote:
> 802.3ad bonding inside a bridge is broken again. Originally fixed by
> 43aa1920117801fe9ae3d1fad886b62511e09bee, the bug was re-introduced by
> 1e253c3b8a1aeed51eef6fc366812f219b97de65.
> 
> LACP frames must not have their skb->dev changed by the bridging hook.
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> 
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1514,6 +1514,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
>  		memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
>  	}
>  
> +	/* prevent bridging code from mangling and forwarding LACP frames */
> +	if (bond->params.mode == BOND_MODE_8023AD &&
> +	    skb->protocol == htons(ETH_P_SLOW))
> +		return RX_HANDLER_PASS;
> +
>  	return RX_HANDLER_ANOTHER;
>  }
>  

It seems to me that 1e253c3b8a1aeed51eef6fc366812f219b97de65 is bogus
and should be reverted, rather than worked around by other drivers.  We
shouldn't enable non-conformant forwarding behaviour by default just
because some people find it useful.  The administrator should have to
explicitly enable it.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] bonding: fix bridged bonds in 802.3ad mode

[Jiri Bohac]

On Thu, Apr 21, 2011 at 08:08:19PM +0100, Ben Hutchings wrote:
> It seems to me that 1e253c3b8a1aeed51eef6fc366812f219b97de65 is bogus
> and should be reverted, rather than worked around by other drivers.  We
> shouldn't enable non-conformant forwarding behaviour by default just
> because some people find it useful.  The administrator should have to
> explicitly enable it.

This is what I thought as well. I find it even more awkward to 
make this behaviour dependend on the STP setting of the bridge
(turning on STP works around this bonding problem, btw).

But even if forwarding of link-local frames is made optional in
some way, it will still break bonding when turned on. So unless
this it is completely reverted, we still need some kind of fix
for bonding.

Btw, this could also be fixed by checking for 
skb->protocol == htons(ETH_P_SLOW) directly in the bridging code.
However, I think the wonderful rx_handler infrastructure makes it
much cleaner this way...

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH] bonding: fix bridged bonds in 802.3ad mode

[Benjamin Poirier]

On 21/04/11 03:08 PM, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 20:47 +0200, Jiri Bohac wrote:
>> 802.3ad bonding inside a bridge is broken again. Originally fixed by
>> 43aa1920117801fe9ae3d1fad886b62511e09bee, the bug was re-introduced by
>> 1e253c3b8a1aeed51eef6fc366812f219b97de65.
>>
>> LACP frames must not have their skb->dev changed by the bridging hook.
>>
>> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>>
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1514,6 +1514,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
>>  		memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
>>  	}
>>  
>> +	/* prevent bridging code from mangling and forwarding LACP frames */
>> +	if (bond->params.mode == BOND_MODE_8023AD &&
>> +	    skb->protocol == htons(ETH_P_SLOW))
>> +		return RX_HANDLER_PASS;
>> +
>>  	return RX_HANDLER_ANOTHER;
>>  }
>>  
> 
> It seems to me that 1e253c3b8a1aeed51eef6fc366812f219b97de65 is bogus

You bet, it's rubbish ;)

Any thoughts on how we could support a transparent bridging
configuration without repurposing br->stp_enabled or adding another
option to bridge-utils?

btw, the particular scenario I was trying to address is a virtual
machine bridged to an ethernet interface connected to a switch port with
802.1x enabled.

-Benjamin

> and should be reverted, rather than worked around by other drivers.  We
> shouldn't enable non-conformant forwarding behaviour by default just
> because some people find it useful.  The administrator should have to
> explicitly enable it.
> 
> Ben.
> 

Re: [PATCH] bonding: fix bridged bonds in 802.3ad mode

[Stephen Hemminger]

On Thu, 21 Apr 2011 16:43:12 -0400
Benjamin Poirier <benjamin.poirier@polymtl.ca> wrote:

> On 21/04/11 03:08 PM, Ben Hutchings wrote:
> > On Thu, 2011-04-21 at 20:47 +0200, Jiri Bohac wrote:
> >> 802.3ad bonding inside a bridge is broken again. Originally fixed by
> >> 43aa1920117801fe9ae3d1fad886b62511e09bee, the bug was re-introduced by
> >> 1e253c3b8a1aeed51eef6fc366812f219b97de65.
> >>
> >> LACP frames must not have their skb->dev changed by the bridging hook.
> >>
> >> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> >>
> >> --- a/drivers/net/bonding/bond_main.c
> >> +++ b/drivers/net/bonding/bond_main.c
> >> @@ -1514,6 +1514,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> >>  		memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
> >>  	}
> >>  
> >> +	/* prevent bridging code from mangling and forwarding LACP frames */
> >> +	if (bond->params.mode == BOND_MODE_8023AD &&
> >> +	    skb->protocol == htons(ETH_P_SLOW))
> >> +		return RX_HANDLER_PASS;
> >> +
> >>  	return RX_HANDLER_ANOTHER;
> >>  }
> >>  
> > 
> > It seems to me that 1e253c3b8a1aeed51eef6fc366812f219b97de65 is bogus
> 
> You bet, it's rubbish ;)
> 
> Any thoughts on how we could support a transparent bridging
> configuration without repurposing br->stp_enabled or adding another
> option to bridge-utils?
> 
> btw, the particular scenario I was trying to address is a virtual
> machine bridged to an ethernet interface connected to a switch port with
> 802.1x enabled.
> 
> -Benjamin
> 
> > and should be reverted, rather than worked around by other drivers.  We
> > shouldn't enable non-conformant forwarding behaviour by default just
> > because some people find it useful.  The administrator should have to
> > explicitly enable it.
> > 
> > Ben.
> > 
> 


The IEEE standard says bridge's shouldn't forward link-local addresses.
The problem is that people expect it to.


-- 

Re: [PATCH] bonding: fix bridged bonds in 802.3ad mode

[David Miller]

From: Benjamin Poirier <benjamin.poirier@polymtl.ca>
Date: Thu, 21 Apr 2011 16:43:12 -0400

> On 21/04/11 03:08 PM, Ben Hutchings wrote:
>> It seems to me that 1e253c3b8a1aeed51eef6fc366812f219b97de65 is bogus
> 
> You bet, it's rubbish ;)

I've reverted it from net-2.6 and will send the revert to -stable
too.

Thanks.

Re: [PATCH] bonding: fix bridged bonds in 802.3ad mode

[Stephen Hemminger]

On Thu, 21 Apr 2011 21:19:35 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Benjamin Poirier <benjamin.poirier@polymtl.ca>
> Date: Thu, 21 Apr 2011 16:43:12 -0400
> 
> > On 21/04/11 03:08 PM, Ben Hutchings wrote:
> >> It seems to me that 1e253c3b8a1aeed51eef6fc366812f219b97de65 is bogus
> > 
> > You bet, it's rubbish ;)
> 
> I've reverted it from net-2.6 and will send the revert to -stable
> too.
> 
> Thanks.

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


--