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
