From mboxrd@z Thu Jan 1 00:00:00 1970 From: Toshiaki Makita Subject: Re: [PATCH RFC 2/3] bridge: Prepare for 802.1ad vlan filtering support Date: Sat, 15 Mar 2014 01:18:56 +0900 Message-ID: <1394813936.1758.23.camel@localhost.localdomain> References: <1394439067-10477-1-git-send-email-makita.toshiaki@lab.ntt.co.jp> <1394439067-10477-3-git-send-email-makita.toshiaki@lab.ntt.co.jp> <532098D2.6050504@redhat.com> <1394714017.4078.51.camel@ubuntu-vm-makita> <5321B85B.1020203@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Stephen Hemminger , netdev@vger.kernel.org, bridge@lists.linux-foundation.org To: vyasevic@redhat.com Return-path: In-Reply-To: <5321B85B.1020203@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: bridge-bounces@lists.linux-foundation.org Errors-To: bridge-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org On Thu, 2014-03-13 at 09:53 -0400, Vlad Yasevich wrote: > On 03/13/2014 08:33 AM, Toshiaki Makita wrote: > > On Wed, 2014-03-12 at 13:26 -0400, Vlad Yasevich wrote: > >> On 03/10/2014 04:11 AM, Toshiaki Makita wrote: > >>> This enables a bridge to have vlan protocol informantion and allows vlan > >>> filtering code to take vlan protocols into account. > > ... > >>> @@ -173,16 +174,27 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v, > >>> * ingress frame is considered to belong to this vlan. > >>> */ > >>> *vid = pvid; > >>> - if (likely(err)) > >>> + if (likely(err)) { > >>> /* Untagged Frame. */ > >>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid); > >>> - else > >>> + if (vlan_tx_tag_present(skb)) { > >>> + /* skb->vlan_proto was different from br->vlan_proto */ > >>> + skb_push(skb, ETH_HLEN); > >>> + skb = __vlan_put_tag(skb, skb->vlan_proto, > >>> + vlan_tx_tag_get(skb)); > >>> + if (unlikely(!skb)) > >>> + return false; > >>> + skb_pull(skb, ETH_HLEN); > >>> + skb_reset_mac_len(skb); > >>> + } > >>> + __vlan_hwaccel_put_tag(skb, proto, pvid); > >> > >> So this seems to be handling the case where we had a protocol mis-match. > >> My question is why are we hiding this case behind our inability to > >> fetch the vid from the packet. > >> > >> I think it might be clearer to make the protocol check explicit > >> (at least if we were to continue using the approach of defining > >> the protocol per bridge). > > > > I didn't intend to handle protocol mismatch, but handle the case where > > the vlan_tci we are about to use happens to be already used. > > In this function, it can occur only if the frame is originally tagged > > with another protocol. > > > > However, indeed, we seem to need the check of skb->vlan_proto only at > > ingress. > > So it maybe makes sense to check the vid and the protocol separately. > > > > I'm thinking of changing that code like this. > > > > bool untagged; > > ... > > err = br_vlan_get_tag(skb, vid); > > if (!err) { > > if (skb->vlan_proto != proto) { > > ... > > skb = __vlan_put_tag(...); > > ... > > *vid = 0; > > untagged = true; > > } else { > > untagged = false; > > } > > } else { > > untagged = true; > > } > > > > if (!*vid) { > > ... > > if (likely(untagged)) { > > /* Untagged Frame. */ > > ... > > } else { > > /* Priority-tagged Frame. > > ... > > } > > } > > > >> > >> This code also has a side-effect that it would be permit 802.1ad packets > >> on an 802.1Q bridge and possibly forward such packets encapsulated yet > >> again. > > > > Well, this is an interesting situation. > > But I have no reason to restrict it. > > Users can configure such an environment if they want. > > This is almost like tunnel mode that is available on some switches. > Does it make sense to explicitly permit/restrict it? I haven't found a description to prohibit a 802.1Q bridge from encapsulating S-tagged frames. So I'm permitting it. A C-VLAN component shouldn't recognize S-tag according to 802.1Q-2011 5.5. Therefore, a C-VLAN component behaves regardless of existence of S-tag. I think some switches support stacked 802.1Q tags (not using 802.1ad). This can't be realized by current implementation. It maybe needs an option to insert a new tag even if the received frame is already tagged. BTW, I happen to have noticed that an S-VLAN bridge uses different destination MAC address for STP (802.1Q-2011 8.13.5 and 13.2). I have to consider it and something similar to it (LACPDU, etc). Thanks, Toshiaki Makita