[RFC PATCH net 0/4] Clear rx-vlan-filter feature in DSA when necessary

[RFC PATCH net 0/4] Clear rx-vlan-filter feature in DSA when necessary

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This is a proposal for an alternative solution to the problems presented
by Tobias here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210308150405.3694678-1-tobias@waldekranz.com/

The key difference is that his patch series makes dsa_slave_vlan_rx_add_vid
accept -EOPNOTSUPP and silently transforms it into an error code of 0,
while my patch series avoids calling dsa_slave_vlan_rx_add_vid when it
is not needed.

Note that the series applies on top of this other bugfix:
https://patchwork.kernel.org/project/netdevbpf/patch/20210308135509.3040286-1-olteanv@gmail.com/

Vladimir Oltean (4):
  net: dsa: on 'bridge vlan add', check for 8021q uppers of all bridge
    ports
  net: dsa: prevent hardware forwarding between unbridged 8021q uppers
  net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not
    global
  net: dsa: let drivers state that they need VLAN filtering while
    standalone

 drivers/net/dsa/hirschmann/hellcreek.c |   1 +
 include/net/dsa.h                      |   3 +
 net/dsa/dsa_priv.h                     |   8 +-
 net/dsa/port.c                         |  60 ++++++++++++++-
 net/dsa/slave.c                        | 100 ++++++++++++++++++++-----
 net/dsa/switch.c                       |  20 +++--
 6 files changed, 164 insertions(+), 28 deletions(-)

-- 
2.25.1

[RFC PATCH net 1/4] net: dsa: on 'bridge vlan add', check for 8021q uppers of all bridge ports

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The first (original) blamed patch intended to prevent the existence of
overlapping 8021q uppers of any bridge port when attempting to offload a
bridge VLAN. Unfortunately, it doesn't check the presence of the offending
8021q upper on all bridge ports, just on the one on which the 'bridge
vlan add' command was emitted (and on the other ports in the crosschip
bitmap, i.e. CPU port and DSA links, all irrelevant).

For example, the following setup:

$ ip link add dev br0 type bridge vlan_filtering 1
$ ip link add dev lan0.100 link lan0 type vlan id 100
$ ip link set dev lan0 master br0
$ ip link set dev lan1 master br0
$ bridge vlan add dev lan1 vid 100

should return an error at the last command, because if it doesn't, the
hardware will be configured in an invalid state where forwarding will
take place between traffic belonging to lan0.100 and lan1.

The trouble is that in hardware, all VLANs kinda smell the same, so if
we offload an 8021q upper on top of a bridged port, this will be in no
way different than adding that VLAN with 'bridge vlan add', however from
the perspective of Linux network stack semantics it is - traffic sent to
8021q uppers is 'stolen' from the bridge rx_handler in the software data
path, and does not take part in bridging unless the 8021q upper is
explicitly bridged, therefore we should observe those semantics.

This changes dsa_slave_vlan_check_for_8021q_uppers into a more reusable
dsa_check_bridge_for_overlapping_8021q_uppers, and also drops the bogus
requirement for holding RCU read-side protection: we are already
serialized with potential writers to the netdev adjacency lists because
we are executing under the rtnl_mutex.

The second blamed patch is where this commit actually applies to.

Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
Fixes: 1ce39f0ee8da ("net: dsa: convert denying bridge VLAN with existing 8021q upper to PRECHANGEUPPER")
Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 63ee2cae4d8e..d36e11399626 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -323,23 +323,27 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
-/* Must be called under rcu_read_lock() */
 static int
-dsa_slave_vlan_check_for_8021q_uppers(struct net_device *slave,
-				      const struct switchdev_obj_port_vlan *vlan)
+dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
+					      u16 vid)
 {
-	struct net_device *upper_dev;
-	struct list_head *iter;
-
-	netdev_for_each_upper_dev_rcu(slave, upper_dev, iter) {
-		u16 vid;
+	struct list_head *iter_upper, *iter_lower;
+	struct net_device *upper, *lower;
 
-		if (!is_vlan_dev(upper_dev))
+	netdev_for_each_lower_dev(bridge_dev, lower, iter_lower) {
+		if (!dsa_slave_dev_check(lower))
 			continue;
 
-		vid = vlan_dev_vlan_id(upper_dev);
-		if (vid == vlan->vid)
-			return -EBUSY;
+		netdev_for_each_upper_dev_rcu(lower, upper, iter_upper) {
+			u16 upper_vid;
+
+			if (!is_vlan_dev(upper))
+				continue;
+
+			upper_vid = vlan_dev_vlan_id(upper);
+			if (upper_vid == vid)
+				return -EBUSY;
+		}
 	}
 
 	return 0;
@@ -368,12 +372,11 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	 * the same VID.
 	 */
 	if (br_vlan_enabled(dp->bridge_dev)) {
-		rcu_read_lock();
-		err = dsa_slave_vlan_check_for_8021q_uppers(dev, &vlan);
-		rcu_read_unlock();
+		err = dsa_check_bridge_for_overlapping_8021q_uppers(dp->bridge_dev,
+								    vlan.vid);
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
-					   "Port already has a VLAN upper with this VID");
+					   "Bridge already has a port with a VLAN upper with this VID");
 			return err;
 		}
 	}
-- 
2.25.1

[RFC PATCH net 2/4] net: dsa: prevent hardware forwarding between unbridged 8021q uppers

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Tobias reports that the following set of commands, which bridge two
ports that have 8021q uppers with the same VID, is incorrectly accepted
by DSA as valid:

.100  br0  .100
   \  / \  /
   lan0 lan1

ip link add dev br0 type bridge vlan_filtering 1
ip link add dev lan0.100 link lan0 type vlan id 100
ip link add dev lan1.100 link lan1 type vlan id 100
ip link set dev lan0 master br0
ip link set dev lan1 master br0 # This should fail but doesn't

Again, this is a variation of the same theme of 'all VLANs kinda smell
the same in hardware, you can't tell if they came from 8021q or from the
bridge'. When the base interfaces are bridged, the expectation of the
Linux network stack is that traffic received by other upper interfaces
except the bridge is not captured by the bridge rx_handler, therefore
not subject to forwarding. So the above setup should not do forwarding
for VLAN ID 100, but it does it nonetheless. So it should be denied.

Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h |  6 +++++-
 net/dsa/port.c     | 23 ++++++++++++++++++++++-
 net/dsa/slave.c    | 13 +++++++++----
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d40dfede494c..d6f9b73241b4 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -181,7 +181,8 @@ int dsa_port_enable_rt(struct dsa_port *dp, struct phy_device *phy);
 int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy);
 void dsa_port_disable_rt(struct dsa_port *dp);
 void dsa_port_disable(struct dsa_port *dp);
-int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br);
+int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
+			 struct netlink_ext_ack *extack);
 void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br);
 int dsa_port_lag_change(struct dsa_port *dp,
 			struct netdev_lag_lower_state_info *linfo);
@@ -261,6 +262,9 @@ static inline bool dsa_tree_offloads_netdev(struct dsa_switch_tree *dst,
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
+int dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
+						  struct net_device *skip,
+						  u16 vid);
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
 int dsa_slave_create(struct dsa_port *dp);
 void dsa_slave_destroy(struct net_device *slave_dev);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index c9c6d7ab3f47..0aad5a84361c 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -144,7 +144,8 @@ static void dsa_port_change_brport_flags(struct dsa_port *dp,
 	}
 }
 
-int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
+int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
+			 struct netlink_ext_ack *extack)
 {
 	struct dsa_notifier_bridge_info info = {
 		.tree_index = dp->ds->dst->index,
@@ -152,8 +153,28 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
 		.port = dp->index,
 		.br = br,
 	};
+	struct net_device *slave = dp->slave;
+	struct net_device *upper_dev;
+	struct list_head *iter;
 	int err;
 
+	netdev_for_each_upper_dev_rcu(slave, upper_dev, iter) {
+		u16 vid;
+
+		if (!is_vlan_dev(upper_dev))
+			continue;
+
+		vid = vlan_dev_vlan_id(upper_dev);
+
+		err = dsa_check_bridge_for_overlapping_8021q_uppers(br, slave,
+								    vid);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Configuration would leak VLAN-tagged packets between bridge ports");
+			return err;
+		}
+	}
+
 	/* Notify the port driver to set its configurable flags in a way that
 	 * matches the initial settings of a bridge port.
 	 */
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d36e11399626..0e884fd439f8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -323,9 +323,9 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
-static int
-dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
-					      u16 vid)
+int dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
+						  struct net_device *skip,
+						  u16 vid)
 {
 	struct list_head *iter_upper, *iter_lower;
 	struct net_device *upper, *lower;
@@ -334,6 +334,9 @@ dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
 		if (!dsa_slave_dev_check(lower))
 			continue;
 
+		if (lower == skip)
+			continue;
+
 		netdev_for_each_upper_dev_rcu(lower, upper, iter_upper) {
 			u16 upper_vid;
 
@@ -373,6 +376,7 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	 */
 	if (br_vlan_enabled(dp->bridge_dev)) {
 		err = dsa_check_bridge_for_overlapping_8021q_uppers(dp->bridge_dev,
+								    NULL,
 								    vlan.vid);
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
@@ -1969,7 +1973,8 @@ static int dsa_slave_changeupper(struct net_device *dev,
 
 	if (netif_is_bridge_master(info->upper_dev)) {
 		if (info->linking) {
-			err = dsa_port_bridge_join(dp, info->upper_dev);
+			err = dsa_port_bridge_join(dp, info->upper_dev,
+						   info->info.extack);
 			if (!err)
 				dsa_bridge_mtu_normalization(dp);
 			err = notifier_from_errno(err);
-- 
2.25.1

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

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The blamed patch has removed the driver's ability to return -EOPNOTSUPP
in the .port_vlan_add method when called from .ndo_vlan_rx_add_vid
(unmassaged by DSA, -EOPNOTSUPP is a hard error for vlan_vid_add).
But we have not managed well enough the cases under which .port_vlan_add
is called in the first place, as will be explained below. This was
reported as a problem by Tobias because mv88e6xxx_port_vlan_prepare is
stubborn and only accepts VLANs on bridged ports. That is understandably
so, because standalone mv88e6xxx ports are VLAN-unaware, and VTU entries
are said to be a scarce resource.

Otherwise said, the following fails lamentably on mv88e6xxx:

ip link add br0 type bridge vlan_filtering 1
ip link set lan3 master br0
ip link add link lan10 name lan10.1 type vlan id 1
[485256.724147] mv88e6085 d0032004.mdio-mii:12: p10: hw VLAN 1 already used by port 3 in br0
RTNETLINK answers: Operation not supported

We need to step back and explain that the dsa_slave_vlan_rx_add_vid and
dsa_slave_vlan_rx_kill_vid methods exist for drivers that need the
'rx-vlan-filter: on' feature in ethtool -k, which can be due to any of
the following reasons:

1. vlan_filtering_is_global = true, and some ports are under a
   VLAN-aware bridge while others are standalone, and the standalone
   ports would otherwise drop VLAN-tagged traffic. This is described in
   commit 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid
   implementation").

2. the ports that are under a VLAN-aware bridge should also set this
   feature, for 8021q uppers having a VID not claimed by the bridge.
   In this case, the driver will essentially not even know that the VID
   is coming from the 8021q layer and not the bridge.

3. Hellcreek. This driver needs it because in standalone mode, it uses
   unique VLANs per port to ensure separation. For separation of untagged
   traffic, it uses different PVIDs for each port, and for separation of
   VLAN-tagged traffic, it never accepts 8021q uppers with the same vid
   on two ports.

If a driver does not fall under any of the above 3 categories, there is
no reason why it should advertise the 'rx-vlan-filter' feature, therefore
no reason why it should offload the VLANs added through vlan_vid_add.

This commit fixes the problem by removing the 'rx-vlan-filter' feature
from the slave devices when they operate in standalone mode, and when
they offload a VLAN-unaware bridge. This gives the mv88e6xxx driver what
it wants, since it keeps the 8021q VLANs away from the VTU until VLAN
awareness is enabled (point at which the ports are no longer standalone,
hence the check in mv88e6xxx_port_vlan_prepare passes). And since the
issue predates the existence of the hellcreek driver, case 3 will be
dealt with in a separate patch.

The commit also has the nice side effect that we no longer lie to the
network stack about our VLAN filtering status.

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

Fixes: 9b236d2a69da ("net: dsa: Advertise the VLAN offload netdev ability only if switch supports it")
Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 37 +++++++++++++++++++++++++++++--
 net/dsa/slave.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d6f9b73241b4..6d0058238a0e 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -274,6 +274,8 @@ int dsa_slave_register_notifier(void);
 void dsa_slave_unregister_notifier(void);
 void dsa_slave_setup_tagger(struct net_device *slave);
 int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
+int dsa_slave_manage_vlan_filtering(struct net_device *dev,
+				    bool vlan_filtering);
 
 static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
 {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 0aad5a84361c..deb3586c0e61 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -384,6 +384,7 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct netlink_ext_ack *extack)
 {
+	bool old_vlan_filtering = dsa_port_is_vlan_filtering(dp);
 	struct dsa_switch *ds = dp->ds;
 	bool apply;
 	int err;
@@ -409,12 +410,44 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 	if (err)
 		return err;
 
-	if (ds->vlan_filtering_is_global)
+	if (ds->vlan_filtering_is_global) {
+		int port;
+
+		for (port = 0; port < ds->num_ports; port++) {
+			struct net_device *slave;
+
+			if (!dsa_is_user_port(ds, port))
+				continue;
+
+			/* We might be called in the unbind path, so not
+			 * all slave devices might still be registered.
+			 */
+			slave = dsa_to_port(ds, port)->slave;
+			if (!slave)
+				continue;
+
+			err = dsa_slave_manage_vlan_filtering(slave,
+							      vlan_filtering);
+			if (err)
+				goto restore;
+		}
+
 		ds->vlan_filtering = vlan_filtering;
-	else
+	} else {
+		err = dsa_slave_manage_vlan_filtering(dp->slave,
+						      vlan_filtering);
+		if (err)
+			goto restore;
+
 		dp->vlan_filtering = vlan_filtering;
+	}
 
 	return 0;
+
+restore:
+	ds->ops->port_vlan_filtering(ds, dp->index, old_vlan_filtering, NULL);
+
+	return err;
 }
 
 /* This enforces legacy behavior for switch drivers which assume they can't
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 0e884fd439f8..e5ca29df7605 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1380,6 +1380,58 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 	return 0;
 }
 
+static int dsa_slave_restore_vlan(struct net_device *vdev, int vid, void *arg)
+{
+	return dsa_slave_vlan_rx_add_vid(arg, vlan_dev_vlan_proto(vdev), vid);
+}
+
+static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
+{
+	return dsa_slave_vlan_rx_kill_vid(arg, vlan_dev_vlan_proto(vdev), vid);
+}
+
+/* Keep the VLAN RX filtering list originating from 8021q uppers in sync with
+ * the hardware only if VLAN filtering is enabled.
+ *
+ * - Standalone ports offload:
+ *   - no VLAN (any 8021q upper is a software VLAN) if
+ *     ds->vlan_filtering_is_global = false
+ *   - the 8021q upper VLANs if ds->vlan_filtering_is_global = true and there
+ *     are bridges spanning this switch chip which have vlan_filtering=1
+ *
+ * - Ports under a vlan_filtering=0 bridge offload:
+ *   - no VLAN if ds->configure_vlan_while_not_filtering = false (deprecated)
+ *   - the bridge VLANs if ds->configure_vlan_while_not_filtering = true
+ *
+ * - Ports under a vlan_filtering=1 bridge offload:
+ *   - the bridge VLANs
+ *   - the 8021q upper VLANs
+ */
+int dsa_slave_manage_vlan_filtering(struct net_device *slave,
+				    bool vlan_filtering)
+{
+	int err;
+
+	if (vlan_filtering) {
+		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+
+		err = vlan_for_each(slave, dsa_slave_restore_vlan, slave);
+		if (err) {
+			vlan_for_each(slave, dsa_slave_clear_vlan, slave);
+			slave->features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
+			return err;
+		}
+	} else {
+		err = vlan_for_each(slave, dsa_slave_clear_vlan, slave);
+		if (err)
+			return err;
+
+		slave->features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
+	}
+
+	return 0;
+}
+
 struct dsa_hw_port {
 	struct list_head list;
 	struct net_device *dev;
@@ -1850,8 +1902,6 @@ int dsa_slave_create(struct dsa_port *port)
 		return -ENOMEM;
 
 	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
-	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
-		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 	slave_dev->hw_features |= NETIF_F_HW_TC;
 	slave_dev->features |= NETIF_F_LLTX;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
-- 
2.25.1

[RFC PATCH net 4/4] net: dsa: let drivers state that they need VLAN filtering while standalone

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

As explained in the blamed patch, the hellcreek driver uses some tricks
to comply with the network stack expectations: it enforces port
separation in standalone mode using VLANs. For untagged traffic,
bridging between ports is prevented by using different PVIDs, and for
VLAN-tagged traffic, it never accepts 8021q uppers with the same VID on
two ports, so packets with one VLAN cannot leak from one port to another.

That is almost fine*, and has worked because hellcreek relied on an
implicit behavior of the DSA core that was changed by the previous
patch: the standalone ports declare the 'rx-vlan-filter' feature as 'on
[fixed]'. Since most of the DSA drivers are actually VLAN-unaware in
standalone mode, that feature was actually incorrectly reflecting the
hardware/driver state, so there was a desire to fix it. This leaves the
hellcreek driver in a situation where it has to explicitly request this
behavior from the DSA framework.

We configure the ports as follows:

- Standalone: 'rx-vlan-filter' is on. An 8021q upper on top of a
  standalone hellcreek port will go through dsa_slave_vlan_rx_add_vid
  and will add a VLAN to the hardware tables, giving the driver the
  opportunity to refuse it through .port_prechangeupper.

- Bridged with vlan_filtering=0: 'rx-vlan-filter' is off. An 8021q upper
  on top of a bridged hellcreek port will not go through
  dsa_slave_vlan_rx_add_vid, because there will not be any attempt to
  offload this VLAN. The driver already disables VLAN awareness, so that
  upper should receive the traffic it needs.

- Bridged with vlan_filtering=1: 'rx-vlan-filter' is on. An 8021q upper
  on top of a bridged hellcreek port will call dsa_slave_vlan_rx_add_vid,
  and can again be vetoed through .port_prechangeupper.

*It is not actually completely fine, because if I follow through
correctly, we can have the following situation:

ip link add br0 type bridge vlan_filtering 0
ip link set lan0 master br0 # lan0 now becomes VLAN-unaware
ip link set lan0 nomaster # lan0 fails to become VLAN-aware again, therefore breaking isolation

This patch fixes that by extending the DSA core logic, based on this
requested attribute, to change the VLAN awareness state of the switch
(port) when it leaves the bridge.

Fixes: e358bef7c392 ("net: dsa: Give drivers the chance to veto certain upper devices")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/hirschmann/hellcreek.c |  1 +
 include/net/dsa.h                      |  3 +++
 net/dsa/slave.c                        |  8 ++++++--
 net/dsa/switch.c                       | 20 +++++++++++++++-----
 4 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 463137c39db2..b8112de24b3f 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -1126,6 +1126,7 @@ static int hellcreek_setup(struct dsa_switch *ds)
 	 * filtering setups are not supported.
 	 */
 	ds->vlan_filtering_is_global = true;
+	ds->needs_standalone_vlan_filtering = true;
 
 	/* Intercept _all_ PTP multicast traffic */
 	ret = hellcreek_setup_fdb(hellcreek);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 83a933e563fe..58ce8089af9a 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -355,6 +355,9 @@ struct dsa_switch {
 	 */
 	bool			vlan_filtering_is_global;
 
+	/* Keep VLAN filtering enabled on unbridged ports. */
+	bool			needs_standalone_vlan_filtering;
+
 	/* Pass .port_vlan_add and .port_vlan_del to drivers even for bridges
 	 * that have vlan_filtering=0. All drivers should ideally set this (and
 	 * then the option would get removed), but it is unknown whether this
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e5ca29df7605..c878fedb81d5 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1395,9 +1395,11 @@ static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
  *
  * - Standalone ports offload:
  *   - no VLAN (any 8021q upper is a software VLAN) if
- *     ds->vlan_filtering_is_global = false
+ *     ds->vlan_filtering_is_global = false and
+ *     ds->needs_standalone_vlan_filtering = false
  *   - the 8021q upper VLANs if ds->vlan_filtering_is_global = true and there
- *     are bridges spanning this switch chip which have vlan_filtering=1
+ *     are bridges spanning this switch chip which have vlan_filtering=1, or
+ *     ds->needs_standalone_vlan_filtering = true.
  *
  * - Ports under a vlan_filtering=0 bridge offload:
  *   - no VLAN if ds->configure_vlan_while_not_filtering = false (deprecated)
@@ -1903,6 +1905,8 @@ int dsa_slave_create(struct dsa_port *port)
 
 	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
 	slave_dev->hw_features |= NETIF_F_HW_TC;
+	if (ds->needs_standalone_vlan_filtering)
+		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 	slave_dev->features |= NETIF_F_LLTX;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	if (!IS_ERR_OR_NULL(port->mac))
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 56ed31b0e636..9a02d24739e1 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -104,9 +104,10 @@ static int dsa_switch_bridge_join(struct dsa_switch *ds,
 static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 				   struct dsa_notifier_bridge_info *info)
 {
-	bool unset_vlan_filtering = br_vlan_enabled(info->br);
 	struct dsa_switch_tree *dst = ds->dst;
 	struct netlink_ext_ack extack = {0};
+	bool change_vlan_filtering = false;
+	bool vlan_filtering;
 	int err, port;
 
 	if (dst->index == info->tree_index && ds->index == info->sw_index &&
@@ -119,6 +120,15 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 						info->sw_index, info->port,
 						info->br);
 
+	if (ds->needs_standalone_vlan_filtering && !br_vlan_enabled(info->br)) {
+		change_vlan_filtering = true;
+		vlan_filtering = true;
+	} else if (!ds->needs_standalone_vlan_filtering &&
+		   br_vlan_enabled(info->br)) {
+		change_vlan_filtering = true;
+		vlan_filtering = false;
+	}
+
 	/* If the bridge was vlan_filtering, the bridge core doesn't trigger an
 	 * event for changing vlan_filtering setting upon slave ports leaving
 	 * it. That is a good thing, because that lets us handle it and also
@@ -127,21 +137,21 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 	 * vlan_filtering callback is only when the last port the last
 	 * VLAN-aware bridge.
 	 */
-	if (unset_vlan_filtering && ds->vlan_filtering_is_global) {
+	if (change_vlan_filtering && ds->vlan_filtering_is_global) {
 		for (port = 0; port < ds->num_ports; port++) {
 			struct net_device *bridge_dev;
 
 			bridge_dev = dsa_to_port(ds, port)->bridge_dev;
 
 			if (bridge_dev && br_vlan_enabled(bridge_dev)) {
-				unset_vlan_filtering = false;
+				change_vlan_filtering = false;
 				break;
 			}
 		}
 	}
-	if (unset_vlan_filtering) {
+	if (change_vlan_filtering) {
 		err = dsa_port_vlan_filtering(dsa_to_port(ds, info->port),
-					      false, &extack);
+					      vlan_filtering, &extack);
 		if (extack._msg)
 			dev_err(ds->dev, "port %d: %s\n", info->port,
 				extack._msg);
-- 
2.25.1

Re: [RFC PATCH net 2/4] net: dsa: prevent hardware forwarding between unbridged 8021q uppers

[Tobias Waldekranz]

On Tue, Mar 09, 2021 at 04:16, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Tobias reports that the following set of commands, which bridge two
> ports that have 8021q uppers with the same VID, is incorrectly accepted
> by DSA as valid:
>
> .100  br0  .100
>    \  / \  /
>    lan0 lan1
>
> ip link add dev br0 type bridge vlan_filtering 1
> ip link add dev lan0.100 link lan0 type vlan id 100
> ip link add dev lan1.100 link lan1 type vlan id 100

If I move this line...

> ip link set dev lan0 master br0
> ip link set dev lan1 master br0 # This should fail but doesn't

...down here, the config is (erroneously) accepted.

> Again, this is a variation of the same theme of 'all VLANs kinda smell
> the same in hardware, you can't tell if they came from 8021q or from the
> bridge'. When the base interfaces are bridged, the expectation of the
> Linux network stack is that traffic received by other upper interfaces
> except the bridge is not captured by the bridge rx_handler, therefore
> not subject to forwarding. So the above setup should not do forwarding
> for VLAN ID 100, but it does it nonetheless. So it should be denied.
>
> Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
> Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

This is what I meant by having bits and pieces of this validation
scattered in multiple places, some things being checked for certain
events but not for others, etc.

I took an initial stab at this to show what I mean:

https://lore.kernel.org/netdev/20210309184244.1970173-1-tobias@waldekranz.com

I am sure there are holes in this as well, hence RFC, but I think it
will be much easier to make sure that we avoid ordering issues using a
structure like this.

What do you think?