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?
-vlad
>
> Thanks,
> Toshiaki Makita
>
>
