Re: [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Mar 22, 2021 at 07:40:27PM -0700, Florian Fainelli wrote:
> On 3/20/2021 3:59 PM, Vladimir Oltean wrote:
> > Because the 'rx-vlan-filter' feature is now dynamically toggled, and our
> > .ndo_vlan_rx_add_vid does not get called when 'rx-vlan-filter' is off,
> > we need to avoid bugs such as the following by replaying the VLANs from
> > 8021q uppers every time we enable VLAN filtering:
> > 
> > ip link add link lan0 name lan0.100 type vlan id 100
> > ip addr add 192.168.100.1/24 dev lan0.100
> > ping 192.168.100.2 # should work
> > ip link add br0 type bridge vlan_filtering 0
> > ip link set lan0 master br0
> > ping 192.168.100.2 # should still work
> > ip link set br0 type bridge vlan_filtering 1
> > ping 192.168.100.2 # should still work but doesn't
> 
> That example seems to work well but see caveat below.
> 
> # ip link add link gphy name gphy.42 type vlan id 42
> # ip addr add 192.168.42.1/24 dev gphy.42
> # ping -c 1 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.473 ms
> 
> --- 192.168.42.254 ping statistics ---
> 1 packets transmitted, 1 packets received, 0% packet loss
> round-trip min/avg/max = 1.473/1.473/1.473 ms
> # ip link add br0 type bridge vlan_filtering 0
> # ip link set br0 up
> # ip addr flush dev gphy
> # ip link set gphy master br0
> [  102.184169] br0: port 1(gphy) entered blocking state
> [  102.189533] br0: port 1(gphy) entered disabled state
> [  102.196039] device gphy entered promiscuous mode
> [  102.200831] device eth0 entered promiscuous mode
> [  102.206781] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
> [  102.214684] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0001, untag: 0x0001
> [  102.228912] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
> [  102.236736] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0101, untag: 0x0001
> [  102.248062] br0: port 1(gphy) entered blocking state
> [  102.253210] br0: port 1(gphy) entered forwarding state

So far so good, the call path below triggers your print for the user
port and the CPU port:
dsa_switch_vlan_add
-> b53_vlan_add
   -> b53_vlan_prepare
      -> b53_enable_vlan
VLAN 42 is not installed in hardware.

> # udhcpc -i br0
> udhcpc: started, v1.32.0
> udhcpc: sending discover
> udhcpc: sending select for 192.168.1.10
> udhcpc: lease of 192.168.1.10 obtained, lease time 600
> deleting routers
> adding dns 192.168.1.254
> # ping 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.294 ms
> 64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.884 ms
> ^C
> --- 192.168.42.254 ping statistics ---
> 2 packets transmitted, 2 packets received, 0% packet loss
> round-trip min/avg/max = 0.884/1.089/1.294 ms
> # ip link set br0 type bridge vlan_filtering 1
> [  116.072754] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 1

Again, so far so good:
dsa_port_vlan_filtering
-> b53_vlan_filtering
   -> b53_enable_vlan(dev->vlan_enabled(was true), filtering(is true))

> [  116.080522] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0

This is where it starts to go downhill. There is a time window inside
dsa_port_vlan_filtering, after we called ds->ops->port_vlan_filtering,
in which we have not yet committed ds->vlan_filtering, yet we still need
to call dsa_slave_manage_vlan_filtering, which may delete or restore
VLANs corresponding to 8021q uppers.

So this happens:
dsa_port_vlan_filtering
-> dsa_slave_manage_vlan_filtering
   -> dsa_slave_restore_vlan
      -> dsa_switch_vlan_add
         -> b53_vlan_add
            -> b53_vlan_prepare
               -> b53_enable_vlan(vlan_enabled(is true), ds->vlan_filtering(is false because it hasn't been committed yet))

I did not take into account the fact that someone might look in
ds->vlan_filtering in port_vlan_add.

> [  116.088211] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0001, untag: 0x0000
> [  116.098696] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
> [  116.106474] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0101, untag: 0x0000

The VLANs are at least restored as expected, it seems.

> # ping 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=0.751 ms
> 64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.700 ms
> ^C
> --- 192.168.42.254 ping statistics ---
> 2 packets transmitted, 2 packets received, 0% packet loss
> round-trip min/avg/max = 0.700/0.725/0.751 ms
> # ping 192.168.1.254
> PING 192.168.1.254 (192.168.1.254): 56 data bytes
> 64 bytes from 192.168.1.254: seq=0 ttl=64 time=0.713 ms
> 64 bytes from 192.168.1.254: seq=1 ttl=64 time=0.916 ms
> 64 bytes from 192.168.1.254: seq=2 ttl=64 time=0.631 ms
> ^C
> --- 192.168.1.254 ping statistics ---
> 3 packets transmitted, 3 packets received, 0% packet loss
> round-trip min/avg/max = 0.631/0.753/0.916 ms
> 
> But you will notice that vlan filtering was not enabled at the switch
> level for a reason I do not fully understand, or rather there were
> multiple calls to port_vlan_filtering with vlan_filtering = 0 for the
> same port.
> 
> Now if we have a nother port that is a member of a bridge that was
> created with vlan_filtering=1 from the get go, the standalone ports are
> not working if they are created before the bridge is:
> 
> # ip link add link gphy name gphy.42 type vlan id 42

VLAN filtering is not enabled, so the VLAN is not installed to hardware,
all ok.

> # ip addr add 192.168.42.1/24 dev gphy.42
> # ping -c 1 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.129 ms
> 
> --- 192.168.42.254 ping statistics ---
> 1 packets transmitted, 1 packets received, 0% packet loss
> round-trip min/avg/max = 1.129/1.129/1.129 ms
> # ip link add br0 type bridge vlan_filtering 1
> # ip link set rgmii_1 master br0
> [   86.835014] br0: port 1(rgmii_1) entered blocking state
> [   86.840622] br0: port 1(rgmii_1) entered disabled state
> [   86.848084] device rgmii_1 entered promiscuous mode
> [   86.853153] device eth0 entered promiscuous mode
> [   86.858308] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1, filtering: 1

So far so good, we have this same code path again:

dsa_port_vlan_filtering
-> b53_vlan_filtering
   -> b53_enable_vlan(dev->vlan_enabled(was true), filtering(is true))

> [   86.866157] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
> [   86.873985] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0001, untag: 0x0000
> [   86.884946] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
> [   86.892879] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0101, untag: 0x0000

Again, we have the same code path that calls dsa_slave_manage_vlan_filtering
while ds->vlan_filtering is still uncommitted, and therefore false. The
b53 driver incorrectly saves this value.

> [   86.904274] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1, filtering: 1
> [   86.912097] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0002, untag: 0x0002
> [   86.925899] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 1
> [   86.933806] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0102, untag: 0x0002

And here we have the bridge pvid installed on the user port and the CPU
port. Since ds->vlan_filtering has been committed in the meantime,
b53_vlan_enable was called again and filtering is now enabled.

> # ip link set br0 up
> [   89.775094] br0: port 1(rgmii_1) entered blocking state
> [   89.780694] br0: port 1(rgmii_1) entered forwarding state
> # ip addr add 192.168.4.10/24 dev br0
> # ping 192.168.4.254
> PING 192.168.4.254 (192.168.4.254): 56 data bytes
> 64 bytes from 192.168.4.254: seq=0 ttl=64 time=1.693 ms
> ^C
> --- 192.168.4.254 ping statistics ---
> 1 packets transmitted, 1 packets received, 0% packet loss
> round-trip min/avg/max = 1.693/1.693/1.693 ms

Pinging through the VLAN-aware bridge, which uses VID 1, works, ok.

> # ping 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> ^C
> --- 192.168.42.254 ping statistics ---
> 2 packets transmitted, 0 packets received, 100% packet loss

Pinging through gphy.42 doesn't work, even though VID 42 was added both
to port 8 and to port 0. I don't understand why. I looked at the b53
driver and I don't see any logic that would skip installing a VLAN if
ds->vlan_filtering is false.

> # ping 192.168.1.254
> PING 192.168.1.254 (192.168.1.254): 56 data bytes
> ^C
> --- 192.168.1.254 ping statistics ---
> 1 packets transmitted, 0 packets received, 100% packet loss
> #

Wait a minute, what interface uses the 192.168.1.0/24 subnet in the
second case?

> 
> Both scenarios work correctly as of net/master prior to this patch series.

So I have no complete idea why it fails either. I do believe DSA does
the right things, for the most part.

Would you be so kind to try this fixup patch on top?

-----------------------------[ cut here ]-----------------------------
From ddca5c56fbf74764977df70c5eba88015bb9832f Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 23 Mar 2021 13:56:24 +0200
Subject: [PATCH] net: dsa: commit vlan_filtering before calling
 dsa_slave_manage_vlan_filtering

Some drivers such as b53 look at ds->vlan_filtering in .port_vlan_add.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/port.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 902095f04e0a..d291e0495084 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -392,6 +392,8 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 	if (ds->vlan_filtering_is_global) {
 		int port;
 
+		ds->vlan_filtering = vlan_filtering;
+
 		for (port = 0; port < ds->num_ports; port++) {
 			struct net_device *slave;
 
@@ -410,15 +412,13 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			if (err)
 				goto restore;
 		}
-
-		ds->vlan_filtering = vlan_filtering;
 	} else {
+		dp->vlan_filtering = vlan_filtering;
+
 		err = dsa_slave_manage_vlan_filtering(dp->slave,
 						      vlan_filtering);
 		if (err)
 			goto restore;
-
-		dp->vlan_filtering = vlan_filtering;
 	}
 
 	return 0;
@@ -426,6 +426,11 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 restore:
 	ds->ops->port_vlan_filtering(ds, dp->index, old_vlan_filtering, NULL);
 
+	if (ds->vlan_filtering_is_global)
+		ds->vlan_filtering = old_vlan_filtering;
+	else
+		dp->vlan_filtering = old_vlan_filtering;
+
 	return err;
 }
 
-----------------------------[ cut here ]-----------------------------

Although I am much less confident now about submitting this as a bugfix
patch to go to stable trees. But I also kind of dislike the idea that
Tobias' patch (which returns -EOPNOTSUPP in dsa_slave_vlan_rx_add_vid)
only masks the problem and makes issues harder to reproduce.

Tobias, how bad is your problem? Do you mind if we tackle it in net-next?
Also, again, any chance you could make mv88e6xxx not refuse the 8021q
VLAN IDs?