Re: [PATCH net 2/4] bridge: Handle priority-tagged frames properly

[Vlad Yasevich <vyasevic@redhat.com>]

On 09/11/2013 03:00 AM, Toshiaki Makita wrote:
> On Tue, 2013-09-10 at 10:03 -0400, Vlad Yasevich wrote:
>> On 09/10/2013 06:34 AM, Toshiaki Makita wrote:
>>> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
>>> use the PVID for the port as its VID.
>>> (See IEEE 802.1Q-2005 6.7.1 and Table 9-2)
>>>
>>> Apply the PVID to not only untagged frames but also priority-tagged frames.
>>>
>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>> ---
>>>    net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
>>>    1 file changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>>> index 21b6d21..5a9c44a 100644
>>> --- a/net/bridge/br_vlan.c
>>> +++ b/net/bridge/br_vlan.c
>>> @@ -189,6 +189,8 @@ out:
>>>    bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>>    			struct sk_buff *skb, u16 *vid)
>>>    {
>>> +	int err;
>>> +
>>>    	/* If VLAN filtering is disabled on the bridge, all packets are
>>>    	 * permitted.
>>>    	 */
>>> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
>>>    	if (!v)
>>>    		return false;
>>>
>>> -	if (br_vlan_get_tag(skb, vid)) {
>>> +	err = br_vlan_get_tag(skb, vid);
>>> +	if (!*vid) {
>>>    		u16 pvid = br_get_pvid(v);
>>>
>>> -		/* Frame did not have a tag.  See if pvid is set
>>> -		 * on this port.  That tells us which vlan untagged
>>> -		 * traffic belongs to.
>>> +		/* Frame had a tag with VID 0 or did not have a tag.
>>> +		 * See if pvid is set on this port.  That tells us which
>>> +		 * vlan untagged or priority-tagged traffic belongs to.
>>>    		 */
>>>    		if (pvid == VLAN_N_VID)
>>>    			return false;
>>>
>>> -		/* PVID is set on this port.  Any untagged ingress
>>> -		 * frame is considered to belong to this vlan.
>>> +		/* PVID is set on this port.  Any untagged or priority-tagged
>>> +		 * ingress frame is considered to belong to this vlan.
>>>    		 */
>>> -		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
>>> +		if (likely(err))
>>> +			/* Untagged Frame. */
>>> +			__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid);
>>> +		else
>>> +			/* Priority-tagged Frame.
>>> +			 * At this point, We know that skb->vlan_tci had
>>> +			 * VLAN_TAG_PRESENT bit and its VID field was 0x000.
>>> +			 * We update only VID field and preserve PCP field.
>>> +			 */
>>> +			skb->vlan_tci |= pvid;
>>> +
>>
>> In the case of a priority tagged frame, we should unroll the
>> modification above and restore the VID field to 0.  Otherwise, you
>> may end up either stripping the vlan header completely or forwarding
>> with pvid of the ingress port.
>
> Thank you for reviewing.
>
> It is my intended behavior that an incoming priority-tagged frame is
> forwarded as a frame untagged or tagged with pvid.
>
> IEEE 802.1Q-2011:
>
>    section 8.1.7 Conversion of frame formats
>
>    NOTE - As all incoming frames, including priority-tagged frames, are
>    classified as belonging to a VLAN, the transmitting Port transmits
>    VLAN-tagged frames or untagged frames. Hence, a station sending a
>    priority-tagged frame via a Bridge will receive a response that is
>    either VLAN-tagged or untagged, as described in 8.5.
>
>    3. Definitions
>
>    3.132 Priority-tagged frame: A tagged frame whose tag header carries
>    priority information but carries no VLAN identification information.
>
>    3.203 VLAN-tagged frame: A VLAN-tagged frame is a tagged frame whose
>    tag header carries *both* VLAN identification and priority
>    information.
>
> Toshiaki Makita
>

Hmm..  The problem is that if a system attached to a port configures a
vlan interface with vid 0 and some priority mappings, then that
interface will not be able to properly receive traffic, as the bridge 
now will never transmit priority tagged frames.

-vlad

>>
>> -vlad
>>>    		return true;
>>>    	}
>>>
>>>
>
>