| Message ID | 20110421184748.GD23756@midget.suse.cz |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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...
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. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
--- 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; }
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>