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

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

[Hervé Gourmelon]

Hello,

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.
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.
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))) {
+		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

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

[Vladimir Oltean]

On Fri, Oct 25, 2024 at 01:46:48PM +0000, Hervé Gourmelon wrote:
> Hello,
> 
> 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.

What does the link partner see? The packet with the 8021.Q tag or
without it?

The packet will always exit the Linux bridge layer as VLAN-tagged,
because skb->offload_fwd_mark will be set*. It will appear with the VLAN
tag in tcpdump, but it should not appear with the VLAN tag on the wire
or on the other side, if the VLAN on the bridge port is egress-untagged.
If you only see this in tcpdump, it is expected behavior and not a problem.

*in turn, that is because we set tx_fwd_offload to true, and the bridge
layer entrusts DSA that it will send the packet into the right VLAN.
Hence the unconditional presence of the tag, and the reliance upon the
port egress setting to strip it in hardware where needed.

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

[Hervé Gourmelon]

Hello Vladimir, and thanks for the answer,

On Fri, Oct 25, 2024 at 02:18:48PM +0000, Vladimir Oltean  wrote:
>> 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.
>
>What does the link partner see? The packet with the 8021.Q tag or
>without it?

The link partner definitely sees the packets with the 802.1Q tags.

>The packet will always exit the Linux bridge layer as VLAN-tagged,
>because skb->offload_fwd_mark will be set*. It will appear with the VLAN
>tag in tcpdump, but it should not appear with the VLAN tag on the wire
>or on the other side, if the VLAN on the bridge port is egress-untagged.
>If you only see this in tcpdump, it is expected behavior and not a problem.

Agreed, and this also what I figured out. But even then, on the link partner, 
I would keep seeing undesired 8021.Q tags in the incoming packets, until
I patched net/dsa/tag_dsa.c as mentioned earlier.

Turning to the Marvell distributor for support (Avnet Silica) I was also given a
hint that the Src_Tagged bit had something to do with the presence of the 
IEEE tag with a 0x8100 Ether type.

Then again, this solution suits my needs (working on a set of 3 cascaded 
MV88E6097 chipsets with an EtherType-DSA link to the CPU) but I won't 
pretend I've tested it in other setups on other Marvell Link Street chipsets...

>*in turn, that is because we set tx_fwd_offload to true, and the bridge
>layer entrusts DSA that it will send the packet into the right VLAN.
>Hence the unconditional presence of the tag, and the reliance upon the
>port egress setting to strip it in hardware where needed.

I tried to figure out that part as well, but couldn't get it to work without 
my patch. I tried to restrict my patch to 'From_CPU' packets at first, but 
I would still occasionally get 'Forward' packets with the unwanted 802.1Q
tag on the link partner.

So in the end I'm sharing this, just in case someone else is stuck with the 
same problem...

Thank you for your time.

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

[Tobias Waldekranz]

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

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

[Hervé Gourmelon]

On fri, oct 25, 2024 at 17:01, Tobias Waldekranz <tobias@waldekranz.com> wrote:
>Hi,

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

Sure.

I'm creating a VLAN-filtering bridge:

            ip link add name br2 type bridge vlan_filtering 1 vlan_default_pvid 0

then adding a number of ports to it (with $itemPort being my variable name for the new ports):

            ip link set $itemPort master br2
            ip link set $itemPort up

then setting up the VLAN on the bridge (with VID = $index_vlan):

            bridge vlan add dev br2 vid $index_vlan self
            bridge vlan global set dev br2 vid $index_vlan
            bridge vlan add dev $itemPort vid $index_vlan pvid untagged


>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.

As i mentioned in my answer to Vladimir, this is not what I experienced. 
I had to reset the Src_Tagged bit for both tags.
But maybe I'm doing something wrong. It's the first time in 12 years 
on that platform that I had to set up an untagged VLAN bridge, so I had
not encountered the problem before.
FYI here is what my DSA looks like (typically, I'm trying to egress untagged traffic on Port0/meth10):
  

                                          +----------+ 
                                Port9(DSA)|          |--->Port0(meth10)  
                                      +---| Switch#1 | 
                                      |   |          | 
                                      |   +----------+ 
 +-------+              +----------+  | 
 |       |   PortA(EDSA)|          |--+Port8(DSA) 
 |  CPU  |--------------| Switch#2 | 
 |       |              |          |--+Port9(DSA) 
 +-------+              +----------+  | 
                                      | 
                                      |   +----------+ 
                                      |   |          | 
                                      +---| Switch#3 | 
                                Port9(DSA)|          | 
                                          +----------+  

I hope that helps. Thanks for your time!
Hervé

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

[Tobias Waldekranz]

On fre, okt 25, 2024 at 16:07, Hervé Gourmelon <herve.gourmelon@ekinops.com> wrote:
> On fri, oct 25, 2024 at 17:01, Tobias Waldekranz <tobias@waldekranz.com> wrote:
>>Hi,
>
>>Could you provide the iproute2/bridge commands used to create this
>>bridge?
>
> Sure.
>
> I'm creating a VLAN-filtering bridge:
>
>             ip link add name br2 type bridge vlan_filtering 1 vlan_default_pvid 0
>
> then adding a number of ports to it (with $itemPort being my variable name for the new ports):
>
>             ip link set $itemPort master br2
>             ip link set $itemPort up
>
> then setting up the VLAN on the bridge (with VID = $index_vlan):
>
>             bridge vlan add dev br2 vid $index_vlan self
>             bridge vlan global set dev br2 vid $index_vlan
>             bridge vlan add dev $itemPort vid $index_vlan pvid untagged
>

Alright, nothing out of the ordinary there.

You say that you are "trying to egress untagged traffic on Port0/meth10"
- could you explain how you do that? br2 is a tagged member of the VLAN
in question, so I guess you have stacked a vlan device on top of it?

In your response to Vladimir, you said that you "occasionally" see
packets egress with unexpected tags. Could you give some examples of
flows that work as expected, and flows that have the errant tags?