Date:   Tue,  9 Mar 2021 19:42:44 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     davem@...emloft.net, kuba@...nel.org
Cc:     andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com,
        olteanv@...il.com, netdev@...r.kernel.org
Subject: [RFC net] net: dsa: Centralize validation of VLAN configuration

There are three kinds of events that have an inpact on VLAN
configuration of DSA ports:

- Adding of stacked VLANs
  (ip link add dev swp0.1 link swp0 type vlan id 1)

- Adding of bridged VLANs
  (bridge vlan add dev swp0 vid 1)

- Changes to a bridge's VLAN filtering setting
  (ip link set dev br0 type bridge vlan_filtering 1)

For all of these events, we want to ensure that some invariants are
upheld:

- For hardware where VLAN filtering is a global setting, either all
  bridges must use VLAN filtering, or no bridge can.

- For all filtering bridges, no stacked VLAN on any port may be
  configured on multiple ports.

- For all filtering bridges, no stacked VLAN may be configured in the
  bridge.

Move the validation of these invariants to a central function, and use
it from all sites where these events are handled. This way, we ensure
that all invariants are always checked, avoiding certain configs being
allowed or disallowed depending on the order in which commands are
given.

Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
---

There is still testing left to do on this, but I wanted to send early
in order show what I meant by "generic" VLAN validation in this
discussion:

https://lore.kernel.org/netdev/87mtvdp97q.fsf@waldekranz.com/

This is basically an alternative implementation of 1/4 and 2/4 from
this series by Vladimir:

https://lore.kernel.org/netdev/20210309021657.3639745-1-olteanv@gmail.com/

net/dsa/dsa_priv.h |   4 ++
 net/dsa/port.c     | 167 ++++++++++++++++++++++++++++++++-------------
 net/dsa/slave.c    |  31 +--------
 3 files changed, 125 insertions(+), 77 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 9d4b0e9b1aa1..c88ef5a43612 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -188,6 +188,10 @@ int dsa_port_lag_change(struct dsa_port *dp,
 int dsa_port_lag_join(struct dsa_port *dp, struct net_device *lag_dev,
 		      struct netdev_lag_upper_info *uinfo);
 void dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag_dev);
+bool dsa_port_can_apply_stacked_vlan(struct dsa_port *dp, u16 vid,
+				     struct netlink_ext_ack *extack);
+bool dsa_port_can_apply_bridge_vlan(struct dsa_port *dp, u16 vid,
+				    struct netlink_ext_ack *extack);
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct netlink_ext_ack *extack);
 bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index c9c6d7ab3f47..3bf457d6775d 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -292,72 +292,141 @@ void dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag)
 	dsa_lag_unmap(dp->ds->dst, lag);
 }
 
-/* Must be called under rcu_read_lock() */
-static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
-					      bool vlan_filtering,
-					      struct netlink_ext_ack *extack)
+static int dsa_port_stacked_vids_collect(struct net_device *vdev, int vid,
+					 void *_stacked_vids)
 {
-	struct dsa_switch *ds = dp->ds;
-	int err, i;
+	unsigned long *stacked_vids = _stacked_vids;
+
+	if (test_bit(vid, stacked_vids))
+		return -EBUSY;
 
-	/* VLAN awareness was off, so the question is "can we turn it on".
-	 * We may have had 8021q uppers, those need to go. Make sure we don't
-	 * enter an inconsistent state: deny changing the VLAN awareness state
-	 * as long as we have 8021q uppers.
+	set_bit(vid, stacked_vids);
+	return 0;
+}
+
+static bool dsa_port_can_apply_vlan(struct dsa_port *dp, bool *mod_filter,
+				    u16 *stacked_vid, u16 *br_vid,
+				    struct netlink_ext_ack *extack)
+{
+	struct dsa_switch_tree *dst = dp->ds->dst;
+	unsigned long *stacked_vids = NULL;
+	struct dsa_port *other_dp;
+	bool filter;
+	u16 vid;
+
+	/* If the modification we are validating is not toggling VLAN
+	 * filtering, use the current setting.
 	 */
-	if (vlan_filtering && dsa_is_user_port(ds, dp->index)) {
-		struct net_device *upper_dev, *slave = dp->slave;
-		struct net_device *br = dp->bridge_dev;
-		struct list_head *iter;
+	if (mod_filter)
+		filter = *mod_filter;
+	else
+		filter = dp->bridge_dev && br_vlan_enabled(dp->bridge_dev);
 
-		netdev_for_each_upper_dev_rcu(slave, upper_dev, iter) {
-			struct bridge_vlan_info br_info;
-			u16 vid;
+	/* For cases where enabling/disabling VLAN awareness is global
+	 * to the switch, we need to handle the case where multiple
+	 * bridges span different ports of the same switch device and
+	 * one of them has a different setting than what is being
+	 * requested.
+	 */
+	if (dp->ds->vlan_filtering_is_global) {
+		list_for_each_entry(other_dp, &dst->ports, list) {
+			if (!other_dp->bridge_dev ||
+			    other_dp->bridge_dev == dp->bridge_dev)
+				continue;
 
-			if (!is_vlan_dev(upper_dev))
+			if (br_vlan_enabled(other_dp->bridge_dev) == filter)
 				continue;
 
-			vid = vlan_dev_vlan_id(upper_dev);
-
-			/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
-			 * device, respectively the VID is not found, returning
-			 * 0 means success, which is a failure for us here.
-			 */
-			err = br_vlan_get_info(br, vid, &br_info);
-			if (err == 0) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Must first remove VLAN uppers having VIDs also present in bridge");
-				return false;
-			}
+			NL_SET_ERR_MSG_MOD(extack, "VLAN filtering is a global setting");
+			goto err;
 		}
+
 	}
 
-	if (!ds->vlan_filtering_is_global)
+	if (!filter)
 		return true;
 
-	/* For cases where enabling/disabling VLAN awareness is global to the
-	 * switch, we need to handle the case where multiple bridges span
-	 * different ports of the same switch device and one of them has a
-	 * different setting than what is being requested.
-	 */
-	for (i = 0; i < ds->num_ports; i++) {
-		struct net_device *other_bridge;
+	stacked_vids = bitmap_zalloc(VLAN_N_VID, GFP_KERNEL);
+	if (!stacked_vids) {
+		WARN_ON_ONCE(1);
+		goto err;
+	}
 
-		other_bridge = dsa_to_port(ds, i)->bridge_dev;
-		if (!other_bridge)
+	/* If the current operation is to add a stacked VLAN, mark it
+	 * as busy. */
+	if (stacked_vid)
+		set_bit(*stacked_vid, stacked_vids);
+
+	/* Forbid any VID used by a stacked VLAN to exist on more than
+	 * one port in the bridge, as the resulting configuration in
+	 * hardware would allow forwarding between those ports. */
+	list_for_each_entry(other_dp, &dst->ports, list) {
+		if (!dsa_is_user_port(other_dp->ds, other_dp->index) ||
+		    !other_dp->bridge_dev ||
+		    other_dp->bridge_dev != dp->bridge_dev)
 			continue;
-		/* If it's the same bridge, it also has same
-		 * vlan_filtering setting => no need to check
-		 */
-		if (other_bridge == dp->bridge_dev)
-			continue;
-		if (br_vlan_enabled(other_bridge) != vlan_filtering) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "VLAN filtering is a global setting");
-			return false;
+
+		if (vlan_for_each(other_dp->slave, dsa_port_stacked_vids_collect,
+				  stacked_vids)) {
+			NL_SET_ERR_MSG_MOD(extack, "Two bridge ports cannot be "
+					   "the base interfaces for VLAN "
+					   "interfaces using the same VID");
+			goto err;
 		}
 	}
+
+	/* If the current operation is to add a bridge VLAN, make sure
+	 * that it is not used by a stacked VLAN. */
+	if (br_vid && test_bit(*br_vid, stacked_vids)) {
+		NL_SET_ERR_MSG_MOD(extack, "A bridge cannot use the same VID "
+				   "already in use by a VLAN interface "
+				   "configured on a bridge port");
+		goto err;
+	}
+
+	/* Ensure that no stacked VLAN is also configured on the bridge
+	 * offloaded by dp as that could result in leakage between
+	 * non-bridged ports. */
+	for_each_set_bit(vid, stacked_vids, VLAN_N_VID) {
+		struct bridge_vlan_info br_info;
+
+		if (br_vlan_get_info(dp->bridge_dev, vid, &br_info))
+			/* Error means that the VID does not exist,
+			 * which is what we want to ensure. */
+			continue;
+
+		NL_SET_ERR_MSG_MOD(extack, "A VLAN interface cannot use a VID "
+				   "that is already in use by a bridge");
+		goto err;
+	}
+
+	kfree(stacked_vids);
 	return true;
+
+err:
+	if (stacked_vids)
+		kfree(stacked_vids);
+	return false;
+}
+
+bool dsa_port_can_apply_stacked_vlan(struct dsa_port *dp, u16 vid,
+				     struct netlink_ext_ack *extack)
+{
+	return dsa_port_can_apply_vlan(dp, NULL, &vid, NULL, extack);
+}
+
+bool dsa_port_can_apply_bridge_vlan(struct dsa_port *dp, u16 vid,
+				    struct netlink_ext_ack *extack)
+{
+	return dsa_port_can_apply_vlan(dp, NULL, NULL, &vid, extack);
+}
+
+static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
+					      bool vlan_filtering,
+					      struct netlink_ext_ack *extack)
+{
+	return dsa_port_can_apply_vlan(dp, &vlan_filtering,
+				       NULL, NULL, extack);
 }
 
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 992fcab4b552..fc0dfeb6b64c 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -363,19 +363,8 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 
 	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
 
-	/* Deny adding a bridge VLAN when there is already an 802.1Q upper with
-	 * 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();
-		if (err) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Port already has a VLAN upper with this VID");
-			return err;
-		}
-	}
+	if (!dsa_port_can_apply_bridge_vlan(dp, vlan.vid, extack))
+		return -EBUSY;
 
 	err = dsa_port_vlan_add(dp, &vlan, extack);
 	if (err)
@@ -2083,28 +2072,14 @@ dsa_slave_check_8021q_upper(struct net_device *dev,
 			    struct netdev_notifier_changeupper_info *info)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct net_device *br = dp->bridge_dev;
-	struct bridge_vlan_info br_info;
 	struct netlink_ext_ack *extack;
-	int err = NOTIFY_DONE;
 	u16 vid;
 
-	if (!br || !br_vlan_enabled(br))
-		return NOTIFY_DONE;
-
 	extack = netdev_notifier_info_to_extack(&info->info);
 	vid = vlan_dev_vlan_id(info->upper_dev);
 
-	/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
-	 * device, respectively the VID is not found, returning
-	 * 0 means success, which is a failure for us here.
-	 */
-	err = br_vlan_get_info(br, vid, &br_info);
-	if (err == 0) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "This VLAN is already configured by the bridge");
+	if (!dsa_port_can_apply_stacked_vlan(dp, vid, extack))
 		return notifier_from_errno(-EBUSY);
-	}
 
 	return NOTIFY_DONE;
 }
-- 
2.25.1

Powered by blists - more mailing lists