Re: [PATCH RFC 2/3] bridge: Prepare for 802.1ad vlan filtering support

[Toshiaki Makita <toshiaki.makita1@gmail.com>]

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