From: Arınç ÜNAL <[email protected]>
When a port is vlan filtering, the VLAN egress type of the CPU port is set
to stack mode. This is so that VLAN tags can be appended after hardware
special tag (called DSA tag in the context of Linux drivers). Because of
this, all frames egress the CPU port VLAN-tagged when vlan filtering is
enabled on a port.
This causes issues with link-local frames, specifically BPDUs, because the
software expects to receive them VLAN-untagged.
Set the egress VLAN tag to consistent for these frames so that they egress
the CPU port exactly as they ingress.
With this change, it can be observed that a bridge interface with stp_state
and vlan_filtering enabled will properly block ports now.
One remaining limitation is that the ingress port must have a PVID assigned
to it for the frame to be trapped to the CPU port. A PVID is set by default
on vlan aware and vlan unaware ports. However, when the network interface
that pertains to the ingress port is attached to a vlan_filtering enabled
bridge, the user can remove the PVID assignment from it which would prevent
the link-local frames from being trapped to the CPU port.
Signed-off-by: Arınç ÜNAL <[email protected]>
---
I couldn't figure out a way to bypass VLAN table lookup for link-local
frames to directly trap them to the CPU port. The CPU port is hardcoded for
MT7530. For MT7531 and the switch on the MT7988 SoC, it depends on the port
matrix to choose the CPU port to trap the frames to. Port matrix and VLAN
table seem to go hand in hand so I don't know if this would even be
possible.
If possible to implement, link-local frames must not be influenced by the
VLAN table. They must always be trapped to the CPU port, and trapped
untagged.
Arınç
---
drivers/net/dsa/mt7530.c | 23 +++++++++++++++--------
drivers/net/dsa/mt7530.h | 8 +++++++-
2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3c1f657593a8..7ff1f8d7e4d6 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1001,16 +1001,23 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
static void
mt753x_trap_frames(struct mt7530_priv *priv)
{
- /* Trap BPDUs to the CPU port(s) */
- mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+ /* Trap 802.1X PAE frames and BPDUs to the CPU port(s) and egress them
+ * exactly as they ingress.
+ */
+ mt7530_rmw(priv, MT753X_BPC, MT753X_PAE_EG_TAG_MASK |
+ MT753X_PAE_PORT_FW_MASK | MT753X_BPDU_EG_TAG_MASK |
+ MT753X_BPDU_PORT_FW_MASK,
+ MT753X_PAE_EG_TAG(MT7530_VLAN_EG_CONSISTENT) |
+ MT753X_PAE_PORT_FW(MT753X_BPDU_CPU_ONLY) |
+ MT753X_BPDU_EG_TAG(MT7530_VLAN_EG_CONSISTENT) |
MT753X_BPDU_CPU_ONLY);
- /* Trap 802.1X PAE frames to the CPU port(s) */
- mt7530_rmw(priv, MT753X_BPC, MT753X_PAE_PORT_FW_MASK,
- MT753X_PAE_PORT_FW(MT753X_BPDU_CPU_ONLY));
-
- /* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
- mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
+ /* Trap LLDP frames with :0E MAC DA to the CPU port(s) and egress them
+ * exactly as they ingress.
+ */
+ mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_EG_TAG_MASK |
+ MT753X_R0E_PORT_FW_MASK,
+ MT753X_R0E_EG_TAG(MT7530_VLAN_EG_CONSISTENT) |
MT753X_R0E_PORT_FW(MT753X_BPDU_CPU_ONLY));
}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 17e42d30fff4..e4e8f2484c25 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -65,12 +65,18 @@ enum mt753x_id {
/* Registers for BPDU and PAE frame control*/
#define MT753X_BPC 0x24
-#define MT753X_BPDU_PORT_FW_MASK GENMASK(2, 0)
+#define MT753X_PAE_EG_TAG_MASK GENMASK(24, 22)
+#define MT753X_PAE_EG_TAG(x) FIELD_PREP(MT753X_PAE_EG_TAG_MASK, x)
#define MT753X_PAE_PORT_FW_MASK GENMASK(18, 16)
#define MT753X_PAE_PORT_FW(x) FIELD_PREP(MT753X_PAE_PORT_FW_MASK, x)
+#define MT753X_BPDU_EG_TAG_MASK GENMASK(8, 6)
+#define MT753X_BPDU_EG_TAG(x) FIELD_PREP(MT753X_BPDU_EG_TAG_MASK, x)
+#define MT753X_BPDU_PORT_FW_MASK GENMASK(2, 0)
/* Register for :03 and :0E MAC DA frame control */
#define MT753X_RGAC2 0x2c
+#define MT753X_R0E_EG_TAG_MASK GENMASK(24, 22)
+#define MT753X_R0E_EG_TAG(x) FIELD_PREP(MT753X_R0E_EG_TAG_MASK, x)
#define MT753X_R0E_PORT_FW_MASK GENMASK(18, 16)
#define MT753X_R0E_PORT_FW(x) FIELD_PREP(MT753X_R0E_PORT_FW_MASK, x)
---
base-commit: 4e192be1a225b7b1c4e315a44754312347628859
change-id: 20240201-b4-for-net-mt7530-fix-link-local-that-ingress-vlan-filtering-ports-6a2099e7ffb3
Best regards,
--
Arınç ÜNAL <[email protected]>
On Thu, Feb 01, 2024 at 10:13:39PM +0300, Arınç ÜNAL via B4 Relay wrote:
> base-commit: 4e192be1a225b7b1c4e315a44754312347628859
> change-id: 20240201-b4-for-net-mt7530-fix-link-local-that-ingress-vlan-filtering-ports-6a2099e7ffb3
>
> Best regards,
> --
> Arınç ÜNAL <[email protected]>
You sent this patch 3 times. What happened, b4 didn't work so well?
On Thu, Feb 01, 2024 at 10:13:39PM +0300, Arınç ÜNAL via B4 Relay wrote:
> One remaining limitation is that the ingress port must have a PVID assigned
> to it for the frame to be trapped to the CPU port. A PVID is set by default
> on vlan aware and vlan unaware ports. However, when the network interface
> that pertains to the ingress port is attached to a vlan_filtering enabled
> bridge, the user can remove the PVID assignment from it which would prevent
> the link-local frames from being trapped to the CPU port.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> I couldn't figure out a way to bypass VLAN table lookup for link-local
> frames to directly trap them to the CPU port. The CPU port is hardcoded for
> MT7530. For MT7531 and the switch on the MT7988 SoC, it depends on the port
> matrix to choose the CPU port to trap the frames to. Port matrix and VLAN
> table seem to go hand in hand so I don't know if this would even be
> possible.
>
> If possible to implement, link-local frames must not be influenced by the
> VLAN table. They must always be trapped to the CPU port, and trapped
> untagged.
Isn't this, in effect, what the "Leaky VLAN Enable" bit does?
On 2.02.2024 01:59, Vladimir Oltean wrote:
> On Thu, Feb 01, 2024 at 10:13:39PM +0300, Arınç ÜNAL via B4 Relay wrote:
>> base-commit: 4e192be1a225b7b1c4e315a44754312347628859
>> change-id: 20240201-b4-for-net-mt7530-fix-link-local-that-ingress-vlan-filtering-ports-6a2099e7ffb3
>>
>> Best regards,
>> --
>> Arınç ÜNAL <[email protected]>
>
> You sent this patch 3 times. What happened, b4 didn't work so well?
Odd, I've received only a single email of this patch.
Looking at lore.kernel.org, it looks like the b4 web endpoint properly
submitted the patch to the b4-sent mailing list but for some reason changed
the Message-Id before submitting it to the netdev mailing list.
This is the email with what the Message-Id should be, which only exists on
the b4-sent mailing list:
https://lore.kernel.org/all/20240201-b4-for-net-mt7530-fix-link-local-that-ingress-vlan-filtering-ports-v1-1-881c1c96b27f@arinc9.com/
This is the email with changed Message-Id that was submitted to the netdev
mailing list:
https://lore.kernel.org/all/=%3Futf-8%3Fq%3F=3C20240201-b4-for-net-mt7530-fix-link-local-that-ingr%3F=%20=%3Futf-8%3Fq%3Fess-vlan-filtering-ports-v1-1-881c1c96b27f=40arinc9=2Ecom=3E%3F=/
There're no brackets enclosing the Message-Id. That must be why Gmail
modified it with the SMTPIN_ADDED_BROKEN disclaimer added for you. I can't
come up with a theory as to why you've received it thrice though.
Konstantin, could you take a look at what happened here?
Arınç
On Fri, Feb 02, 2024 at 12:11:37PM +0300, Arınç ÜNAL wrote:
> There're no brackets enclosing the Message-Id. That must be why Gmail
> modified it with the SMTPIN_ADDED_BROKEN disclaimer added for you. I can't
> come up with a theory as to why you've received it thrice though.
>
> Konstantin, could you take a look at what happened here?
Oh, yay, another python email module bug. *sigh* It's not supposed to break
items enclosed in brackets.
I'll see if I can work around it on the submission side of things, but for now
the silly workaround is not to create branch names that are too long (which
results in message-ids that are very long).
-K
On Fri, Feb 02, 2024 at 01:26:19AM +0200, Vladimir Oltean wrote:
> On Thu, Feb 01, 2024 at 10:13:39PM +0300, Arınç ÜNAL via B4 Relay wrote:
> > One remaining limitation is that the ingress port must have a PVID assigned
> > to it for the frame to be trapped to the CPU port. A PVID is set by default
> > on vlan aware and vlan unaware ports. However, when the network interface
> > that pertains to the ingress port is attached to a vlan_filtering enabled
> > bridge, the user can remove the PVID assignment from it which would prevent
> > the link-local frames from being trapped to the CPU port.
> >
> > Signed-off-by: Arınç ÜNAL <[email protected]>
> > ---
> > I couldn't figure out a way to bypass VLAN table lookup for link-local
> > frames to directly trap them to the CPU port. The CPU port is hardcoded for
> > MT7530. For MT7531 and the switch on the MT7988 SoC, it depends on the port
> > matrix to choose the CPU port to trap the frames to. Port matrix and VLAN
> > table seem to go hand in hand so I don't know if this would even be
> > possible.
> >
> > If possible to implement, link-local frames must not be influenced by the
> > VLAN table. They must always be trapped to the CPU port, and trapped
> > untagged.
>
> Isn't this, in effect, what the "Leaky VLAN Enable" bit does?
Hm? Any news on this? I suppose this was the reason for submitting the
otherwise ok patch as RFC?
On 16.02.2024 04:24, Vladimir Oltean wrote:
> On Fri, Feb 02, 2024 at 01:26:19AM +0200, Vladimir Oltean wrote:
>> On Thu, Feb 01, 2024 at 10:13:39PM +0300, Arınç ÜNAL via B4 Relay wrote:
>>> One remaining limitation is that the ingress port must have a PVID assigned
>>> to it for the frame to be trapped to the CPU port. A PVID is set by default
>>> on vlan aware and vlan unaware ports. However, when the network interface
>>> that pertains to the ingress port is attached to a vlan_filtering enabled
>>> bridge, the user can remove the PVID assignment from it which would prevent
>>> the link-local frames from being trapped to the CPU port.
>>>
>>> Signed-off-by: Arınç ÜNAL <[email protected]>
>>> ---
>>> I couldn't figure out a way to bypass VLAN table lookup for link-local
>>> frames to directly trap them to the CPU port. The CPU port is hardcoded for
>>> MT7530. For MT7531 and the switch on the MT7988 SoC, it depends on the port
>>> matrix to choose the CPU port to trap the frames to. Port matrix and VLAN
>>> table seem to go hand in hand so I don't know if this would even be
>>> possible.
>>>
>>> If possible to implement, link-local frames must not be influenced by the
>>> VLAN table. They must always be trapped to the CPU port, and trapped
>>> untagged.
>>
>> Isn't this, in effect, what the "Leaky VLAN Enable" bit does?
>
> Hm? Any news on this? I suppose this was the reason for submitting the
> otherwise ok patch as RFC?
I was just finalising my response. I wanted to study the switch a little
bit so that I could give a better response than "the leaky VLAN option
doesn't do anything :/".
From what I understand, with the leaky VLAN option enabled, the frame won't
possibly be dropped at the VLAN table lookup procedure. This is useless in
this case because with commit ("net: dsa: mt7530: drop untagged frames on
VLAN-aware ports without PVID"), PVC.ACC_FRM of port without PVID set on
software is set to TAGGED to prevent untagged frames ingressing through
this port from being forwarded. So what we need instead is to allow
untagged frames to be forwarded.
With PVC.ACC_FRM set to ALL, all VLAN-untagged frames will be forwarded
[1]. With that, we need to configure the switch in a way that only the
link-local frames will be forwarded. I have yet to find a way to achieve
this.
Before that, here's how I think the switching procedure works.
VLAN-tagged/VLAN-untagged frame ingresses through port
1. Address Table Learning
The switch will add an entry to the MAC address table; the source
address as ADDRESS, the ingress port as PORT / FILTER, [if tagged
frame and PVC.VLAN_ATTR = USER: VID on the VLAN tag; if untagged frame
(and PVC.VLAN_ATTR = USER or TRANSPARENT), or PVC.VLAN_ATTR =
TRANSPARENT (and untagged or tagged frame): PPBV1.G0_PORT_VID] as CVID.
2. Address Table Lookup Procedure
For unicast frame type, the switch will look up the destination address
as ADDRESS on the MAC address table. If the destination address does not
match a MAC address table entry, the ports set on MFC.UNU_FFP will be
the forwardable ports.
For broadcast frame type, the ports set on MFC.BC_FFP will be the
forwardable ports.
For non-IP-multicast multicast frame type, the ports set on MFC.UNM_FFP
will be the forwardable ports.
3. If PVC.ACC_FRM is set to TAGGED, the switch will drop VLAN-untagged
frames.
4. If PCR.PORT_VLAN is set to PORT_MATRIX mode, skip to step 6.
5. VLAN Table Lookup Procedure
The switch will look up the VID on the VLAN table. If the frame is
VLAN-untagged, the VID will be PPBV1.G0_PORT_VID of the ingress port. If
entry with the VID does not exist on the VLAN table entry, on SECURITY
and CHECK modes, the switch will drop the frame. On FALLBACK mode, the
switch won't drop the frame.
The switch will look up the ingress port on PORT_MEM on the VLAN table
entry that matches the VID. If the ingress port is not set on PORT_MEM,
on SECURITY mode, the switch will drop the frame. On FALLBACK and CHECK
modes, the switch won't drop the frame.
6. The switch will look up the ports set on PCR.PORT_MATRIX to narrow down
the ports to forward the frame to. The ingress port will be excluded.
7. VLAN Tag Egress Procedure
The EG_TAG bit for frames:
PPPoE Discovery_ARP/RARP: PPP_EG_TAG and ARP_EG_TAG in the APC register.
IGMP_MLD: IGMP_EG_TAG and MLD_EG_TAG in the IMC register.
BPDU and PAE: BPDU_EG_TAG and PAE_EG_TAG in the BPC register.
REV_01 and REV_02: R01_EG_TAG and R02_EG_TAG in the RGAC1 register.
REV_03 and REV_0E: R03_EG_TAG and R0E_EG_TAG in the RGAC2 register.
REV_10 and REV_20: R10_EG_TAG and R20_EG_TAG in the RGAC3 register.
REV_21 and REV_UN: R21_EG_TAG and RUN_EG_TAG in the RGAC4 register.
For other frames, one of these options set the earliest in this order
will apply to the frame:
EG_TAG in the address table.
EG_TAG in the PVC register.
EG_CON and EG_TAG per port in the VLAN table.
EG_TAG in the PCR register.
Frame egresses through port(s)
Clarifications:
- MAC SA of untagged frames are learned even with PVC.ACC_FRM of the
ingress port set to TAGGED. That's why the 3rd step comes after the 1st
step. Untagged frames are still dropped with PCR.PORT_VLAN of the ingress
port set to PORT_MATRIX mode. That's why the 3rd step comes before the
4th step. I can't prove untagged frames are dropped before address table
lookup.
- IP multicast frames are looked up on "Destination IP Address Table", and
"Source IP Address Table" if IGMPv3/MLDv2. I haven't studied these yet so
I didn't explain processing IP multicast frames.
With VLAN-untagged frames forwarded, frames will leak to non-ingress ports
[2].
We can at least egress non-link-local frames tagged [3]. As stated above,
PVC_EG_TAG does not apply to link-local frames.
If we could disable unicast frame forwarding completely, and unknown
(broadcast, multicast) frame flooding for frames with certain VID, we could
set PVID to 4095 when there's no PVID on software, and disable them for VID
4095 frames. I don't believe this operation exists on this switch IP.
In conclusion, these are the options:
- Let frames tagged with VID 0 leak to non-ingress ports for the benefit
of always trapping link-local frames.
- Find a way to prevent the leaks.
- Keep disallowing untagged frames from being forwarded, link-local frames
won't always be trapped.
I would love your input as someone who has much more experience than I do.
[1]
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index eab1f52e7eb3..1e160b6eb035 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1699,11 +1699,6 @@ mt7530_port_vlan_add(struct dsa_switch *ds, int port,
/* This VLAN is overwritten without PVID, so unset it */
priv->ports[port].pvid = G0_PORT_VID_DEF;
- /* Only accept tagged frames if the port is VLAN-aware */
- if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
- mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
- MT7530_VLAN_ACC_TAGGED);
-
mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
G0_PORT_VID_DEF);
}
With this and the commands below, link-local frames will be trapped to the
CPU port.
ip l add br0 type bridge vlan_filtering 1 && \
ip l set lan1 master br0 && \
ip l set br0 up && \
ip l set lan1 up && \
bridge v a vid 1 dev lan1 && \
tcpdump -X -i eth0
[2]
Enabled ports: P0-P4 as user & P6 as CPU
On VLAN filtering bridge: P0, P1
On VID 0: P0, P1 (no PVID on software)
P2, P3, P4 (standalone)
P6 (CPU port)
Address table:
PORT / FILTER of VID 0 entry ADDRESS PC0: 00000001 (destination port is P0)
PORT / FILTER of VID 0 entry ADDRESS PC1: 00000010 (destination port is P1)
[...]
PORT / FILTER of VID 0 entry ADDRESS SoC: 01000000 (destination port is P6)
VLAN table:
PORT_MEM of VID 0 entry: 01011111
P0 & P1 registers:
PVC.ACC_FRM = ALL
PPBV1.G0_PORT_VID = 0
PCR.PORT_VLAN = SECURITY
PVC.VLAN_ATTR = USER
PCR.PORT_MATRIX = 01000011
VLAN egress configuration:
EG_TAG of VID 0 address table entry ADDRESS PC0 & PC1 = SYSTEM_DEFAULT
P0 & P1 PVC.EG_TAG = SYSTEM_DEFAULT
EG_CON/[EG_TAG per port] of VID 0 VLAN table entry = EG_TAG (TAGGED for P0
& P1, STACK for P6)
P0 & P1 PCR.EG_TAG = SYSTEM_DEFAULT
Global registers:
MFC.BC_FFP = 01011111
MFC.UNU_FFP = 01011111
Broadcast untagged frame ingresses through P0:
1. An entry is added to the MAC address table. P0 as destination port, 0 as
CVID.
2. MFC.BC_FFP is used to set the forwardable ports, 01011111.
3. Untagged frame is allowed.
4. VLAN table is not skipped.
5. VLAN table entry with 0 as VID exists, the frame is allowed. Ingress
port is set on PORT_MEM, the frame is allowed.
6. Bitwise AND with PCR.PORT_MATRIX results in 01000011. Bitwise AND with
~00000001 (ingress port) results in 01000010. Frame egresses through P1
& P6.
7. EG_TAG per port of VID 0 VLAN table entry applies. Frame eggresses
tagged through P1, stacked through P6.
Unicast untagged frame with destination address PC1 ingresses through P0:
1. An entry is added to the MAC address table. P0 as destination port, 0 as
CVID.
2. Destination address matches an entry. PORT / FILTER of the matching
entry is used to set the forwardable ports, 00000010.
3. Untagged frame is allowed.
4. VLAN table is not skipped.
5. VLAN table entry with 0 as VID exists, the frame is allowed. Ingress
port is set on PORT_MEM, the frame is allowed.
6. Bitwise AND with PCR.PORT_MATRIX results in 00000010. Bitwise AND with
~00000001 (ingress port) results in 00000010. Frame egresses through P1.
7. EG_TAG per port of VID 0 VLAN table entry applies. Frame eggresses
tagged.
Unicast untagged frame with unknown destination address ingresses through
P0:
1. An entry is added to the MAC address table. P0 as destination port, 0 as
CVID.
2. Destination address doesn't match an entry. MFC.UNU_FFP is used to set
the forwardable ports, 01011111.
3. Untagged frame is allowed.
4. VLAN table is not skipped.
5. VLAN table entry with 0 as VID exists, the frame is allowed. Ingress
port is set on PORT_MEM, the frame is allowed.
6. Bitwise AND with PCR.PORT_MATRIX results in 01000011. Bitwise AND with
~00000001 (ingress port) results in 01000010. Frame egresses through P1
& P6.
7. EG_TAG per port of VID 0 VLAN table entry applies. Frame eggresses
tagged through P1, stacked through P6.
[3]
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index eab1f52e7eb3..2c7c5eaba4b6 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1699,10 +1699,10 @@ mt7530_port_vlan_add(struct dsa_switch *ds, int port,
/* This VLAN is overwritten without PVID, so unset it */
priv->ports[port].pvid = G0_PORT_VID_DEF;
- /* Only accept tagged frames if the port is VLAN-aware */
+ /* Egress leaks tagged */
if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
- mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
- MT7530_VLAN_ACC_TAGGED);
+ mt7530_rmw(priv, MT7530_PVC_P(port), PVC_EG_TAG_MASK,
+ PVC_EG_TAG(MT7530_VLAN_EG_TAGGED));
mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
G0_PORT_VID_DEF);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 3ef9ed0163a1..5ff9a30ef4de 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -259,6 +259,7 @@ enum mt7530_port_mode {
enum mt7530_vlan_port_eg_tag {
MT7530_VLAN_EG_DISABLED = 0,
MT7530_VLAN_EG_CONSISTENT = 1,
+ MT7530_VLAN_EG_TAGGED = 6,
};
enum mt7530_vlan_port_attr {
Arınç