From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: PATCH: fix bridged 802.3ad bonding Date: Wed, 4 Jun 2008 09:06:33 -0700 Message-ID: <20080604090633.2a12cecc@extreme> References: <20080603132106.GA3256@midget.suse.cz> <20080603094604.6a7dfe7d@extreme> <20080603193227.GA4050@midget.suse.cz> <20080603131326.1f70915e@extreme> <18105.1212528128@death> <20080603215519.298c0cd3@extreme> <20080604082425.GA3272@midget.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Jay Vosburgh , Jiri Bohac , netdev@vger.kernel.org, David Miller To: Jiri Bohac Return-path: Received: from smtp1.linux-foundation.org ([140.211.169.13]:54763 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758683AbYFDQMh (ORCPT ); Wed, 4 Jun 2008 12:12:37 -0400 In-Reply-To: <20080604082425.GA3272@midget.suse.cz> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, 4 Jun 2008 10:24:25 +0200 Jiri Bohac wrote: > On Tue, Jun 03, 2008 at 09:55:19PM -0700, Stephen Hemminger wrote: > > I prefer the following because it process all link-local frames through > > the normal input path. This means the frames will: > > * be filterable by netfilter > > Well, the LACP frames are not filtered by netfilter when there is > bonding on its own (not part of a bridge), so I don't see why > this should change when the bond is made part of a bridge. > > Maybe it is a good idea to run the LACP frames through netfilter, > but I think this should be done consistently in the bonding code, > whether or not bridging is set up, and probably on the individual > slave interfaces. It does not make sense to filter bonding's LACP > frames in ebtables, IMHO. > > > * not forwarded across bridge (this is important). > > I thought this was the case with my second patch as well (?) > > > > --- a/net/bridge/br_input.c 2008-06-03 21:44:54.000000000 -0700 > > +++ b/net/bridge/br_input.c 2008-06-03 21:52:20.000000000 -0700 > > @@ -135,15 +135,12 @@ struct sk_buff *br_handle_frame(struct n > > /* Pause frames shouldn't be passed up by driver anyway */ > > if (skb->protocol == htons(ETH_P_PAUSE)) > > goto drop; > > - > > - /* Process STP BPDU's through normal netif_receive_skb() path */ > > - if (p->br->stp_enabled != BR_NO_STP) { > > - if (NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev, > > - NULL, br_handle_local_finish)) > > - return NULL; > > - else > > - return skb; > > - } > > + > > + if (NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev, > > + NULL, br_handle_local_finish)) > > + return NULL; /* frame consumed by filter */ > > + else > > + return skb; /* continue processing */ > > } > > > > switch (p->state) { > > where did the "if (p->br->stp_enabled != BR_NO_STP)" condition > go? Is it not needed? I thought it was there to prevent the STP > BPDUs from being handled when STP is turned off. > That is already done in br_stp_rcv so the check here was not needed.