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

[Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>]

On Wed, 2013-09-11 at 12:32 -0400, Vlad Yasevich wrote:
> 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

I see.  As you say, for example, if we configure vnet0.0 and attach
vnet0 to a bridge with vlan_filtering enabled, even though we are
sending priority-tagged frames from vnet0.0 and they are successfully
forwarded by the bridge to another port, we cannot receive any frame on
vnet0.0 because all incoming frames from a bridge port are not
priority-tagged and never passed to vnet0.0.

I think this might be a problem as an end station that cannot receive
incoming untagged frames and priority-tagged frames on the same
interface when vlan 0 interface is defined.
(We can receive both untagged and priority-tagged frames on the same
interface that is not configured vlan 0.)

If we assume it as a problem of vlan interface, I guess we should enable
vlan 0 interface to receive untagged frames (treat vnet0.0 as an alias
of vnet0), or add a setting of egress priority map to vnet0 so that vlan
0 interface can become unnecessary.

If it is preferable to deal with it by bridge, I don't think a
priority-tagged frame's VID field should be restored to 0 on the egress
port.  If we do so, we may receive both untagged frames and
priority-tagged frames from a bridge port when another end station sends
both untagged and priority-tagged frames by mixture.
IMO, it will be resolved by adding a new per-port policy that enables us
to send priority-tagged frames instead of untagged frames, like the
"priority-tags" option of openvswitch.
This solution, however, makes the bridge not be conformed with IEEE
802.1Q, and the problem remains that we cannot receive any frames on a
vlan 0 interface such as eth0.0 when we connect eth0 to another 802.1Q
conformed switch.

I'd like to hear further comments or suggestions, everyone :)

Thanks,

Toshiaki Makita

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