Re: [PATCH 1/1] net: dsa: fix tag_dsa.c for untagged VLANs

[Tobias Waldekranz <tobias@waldekranz.com>]

On fre, okt 25, 2024 at 13:46, Hervé Gourmelon <herve.gourmelon@ekinops.com> wrote:
> Hello,

Hi,

>
> Trying to set up an untagged VLAN bridge on a DSA architecture of 
> mv88e6xxx switches, I realized that whenever I tried to emit a 
> 'From_CPU' or 'Forward' DSA packet, it would always egress with an 
> unwanted 802.1Q header on the bridge port.

Could you provide the iproute2/bridge commands used to create this
bridge?

As Vladimir pointed out: the bridge will leave the .1Q-tag in the packet
when sending it down to the port netdev, to handle all offloading
scenarios (multiple untagged memberships can not be supported without
this information). tcpdump does not tell you how the packet will look
when it egresses the physical port in this case.

> Taking a closer look at the code, I saw that the Src_Tagged bit of the
> DSA header (1st octet, bit 5) was always set to '1' due to the
> following line:
>
> 	dsa_header[0] = (cmd << 6) | 0x20 | tag_dev;
>
> which is wrong: Src_Tagged should be reset if we need the frame to
> egress untagged from the bridge port.

This only matters for FROM_CPU tags, which contain _destination_
information.

FORWARD tags contain information about how a packet was originally
_received_. When receiving a FORWARD, the switch uses VTU membership
data to determine whether to egress tagged or untagged, per port.

> So I added a few lines to check whether the port is a member of a VLAN
> bridge, and whether the VLAN is set to egress untagged from the port,
> before setting or resetting the Src_Tagged bit as needed.
>
> Signed-off-by: Hervé Gourmelon <herve.gourmelon@ekinops.com>
> ---
>  net/dsa/tag_dsa.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index 2a2c4fb61a65..14b4d8c3dc8a 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -163,6 +163,21 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  	 */
>  	if (skb->protocol == htons(ETH_P_8021Q) &&
>  	    (!br_dev || br_vlan_enabled(br_dev))) {

The only way past this guard is (1) that the packet has a .1Q-tag, and
that either (2a) it is a standalone port, or (2b) it is a port in a VLAN
filtering bridge.

In case 1+2a: We will generate a FROM_CPU, so the tagged bit has
meaning. But since the port is standalone, the tag in the packet is
"real" (not coming from bridge offloading) and should be in the packet
when it hits the wire.

In case 1+2b: (Your case) We will generate a FORWARD, so the tagged bit
does not matter at all.

Does that make sense?

> +		struct bridge_vlan_info br_info;
> +		u16 vid = 0;
> +		u16 src_tagged = 1;
> +		u8 *vid_ptr;
> +		int err = 0;
> +
> +		/* Read VID from VLAN 802.1Q tag */
> +		vid_ptr = dsa_etype_header_pos_tx(skb);
> +		vid = ((vid_ptr[2] & 0x0F) << 8 | vid_ptr[3]);
> +		/* Get VLAN info for vid on net_device *dev (dsa slave) */
> +		err = br_vlan_get_info_rcu(dev, vid, &br_info);
> +		if (err == 0 && (br_info.flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
> +			src_tagged = 0;
> +		}
> +
>  		if (extra) {
>  			skb_push(skb, extra);
>  			dsa_alloc_etype_header(skb, extra);
> @@ -170,11 +185,11 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  
>  		/* Construct tagged DSA tag from 802.1Q tag. */
>  		dsa_header = dsa_etype_header_pos_tx(skb) + extra;
> -		dsa_header[0] = (cmd << 6) | 0x20 | tag_dev;
> +		dsa_header[0] = (cmd << 6) | (src_tagged << 5) | tag_dev;
>  		dsa_header[1] = tag_port << 3;
>  
>  		/* Move CFI field from byte 2 to byte 1. */
> -		if (dsa_header[2] & 0x10) {
> +		if (src_tagged == 1 && dsa_header[2] & 0x10) {
>  			dsa_header[1] |= 0x01;
>  			dsa_header[2] &= ~0x10;
>  		}
> -- 
> 2.34.1