[RFC net] net: dsa: Centralize validation of VLAN configuration

[RFC net] net: dsa: Centralize validation of VLAN configuration

[Tobias Waldekranz]

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

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Florian Fainelli]

On 3/9/21 10:42 AM, Tobias Waldekranz wrote:
> 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.

I suppose that is true, given that a non-VLAN filtering bridge must not
perform ingress VID checking, OK.

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

You need to qualify multiple ports a bit more here, are you saying
multiple ports that are part of said bridge, or?

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

Being stacked in the bridge does not really compute for me, you mean, no
VLAN upper must be configured on the bridge master device(s)? Why would
that be a problem though?

> 
> 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@waldekranz.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/

I really have not been able to keep up with your discussion, and I am
not sure if I will given how quickly you guys can spin patches (not a
criticism, this is welcome).

> 
> 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;
>  }
> 


-- 
Florian

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Tobias Waldekranz]

On Tue, Mar 09, 2021 at 12:40, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 3/9/21 10:42 AM, Tobias Waldekranz wrote:
>> 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.
>
> I suppose that is true, given that a non-VLAN filtering bridge must not
> perform ingress VID checking, OK.
>
>> 
>> - For all filtering bridges, no stacked VLAN on any port may be
>>   configured on multiple ports.
>
> You need to qualify multiple ports a bit more here, are you saying
> multiple ports that are part of said bridge, or?

Yeah sorry, I can imagine that makes no sense whatsoever without the
context of the recent discussions. It is basically guarding against this
situation:

.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

>> - For all filtering bridges, no stacked VLAN may be configured in the
>>   bridge.
>
> Being stacked in the bridge does not really compute for me, you mean, no
> VLAN upper must be configured on the bridge master device(s)? Why would
> that be a problem though?

Again sorry, I relize that this message needs a lot of work. It guards
against this scenario:

.100  br0
   \  / \
   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 set dev lan0 master br0
$ ip link set dev lan1 master br0
$ bridge vlan add dev lan1 vid 100 # This should fail

>> 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@waldekranz.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/
>
> I really have not been able to keep up with your discussion, and I am
> not sure if I will given how quickly you guys can spin patches (not a
> criticism, this is welcome).

Yeah I know, it has been a bit of a whirlwind.

Maybe I should just have posted this inline in the other thread, since
it was mostly to show Vladimir my idea, and it seemed easier to write it
in C than in English :)

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Vladimir Oltean]

On Tue, Mar 09, 2021 at 10:28:11PM +0100, Tobias Waldekranz wrote:
> On Tue, Mar 09, 2021 at 12:40, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 3/9/21 10:42 AM, Tobias Waldekranz wrote:
> >> 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.
> >
> > I suppose that is true, given that a non-VLAN filtering bridge must not
> > perform ingress VID checking, OK.
> >
> >> 
> >> - For all filtering bridges, no stacked VLAN on any port may be
> >>   configured on multiple ports.
> >
> > You need to qualify multiple ports a bit more here, are you saying
> > multiple ports that are part of said bridge, or?
> 
> Yeah sorry, I can imagine that makes no sense whatsoever without the
> context of the recent discussions. It is basically guarding against this
> situation:
> 
> .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
> 
> >> - For all filtering bridges, no stacked VLAN may be configured in the
> >>   bridge.
> >
> > Being stacked in the bridge does not really compute for me, you mean, no
> > VLAN upper must be configured on the bridge master device(s)? Why would
> > that be a problem though?
> 
> Again sorry, I relize that this message needs a lot of work. It guards
> against this scenario:
> 
> .100  br0
>    \  / \
>    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 set dev lan0 master br0
> $ ip link set dev lan1 master br0
> $ bridge vlan add dev lan1 vid 100 # This should fail
> 
> >> 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@waldekranz.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/
> >
> > I really have not been able to keep up with your discussion, and I am
> > not sure if I will given how quickly you guys can spin patches (not a
> > criticism, this is welcome).
> 
> Yeah I know, it has been a bit of a whirlwind.
> 
> Maybe I should just have posted this inline in the other thread, since
> it was mostly to show Vladimir my idea, and it seemed easier to write it
> in C than in English :)

I like it, I think it has good potential.
I wrote up this battery of tests, there is still one condition which you
are not catching, but you should be able to add it. If you find more
corner cases please feel free to add them to this list. Then you can
clean up the patch and send it, I think.

-----------------------------[ cut here ]-----------------------------
From 9fcfccb6a38a9769962b098ba19d50e576710b5b Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 9 Mar 2021 23:51:01 +0200
Subject: [PATCH] selftests: net: dsa: add checks for all VLAN configurations
 known to mankind that should fail

Offloading VLANs from two different directions is no easy feat,
especially since we can toggle the VLAN filtering property at runtime,
and even per port!

Try to capture the combinations of commands that should be rejected by
DSA, in the attempt of creating a validation procedure that catches them
all.

Note that this patch moves the irritating "require_command" for mausezahn
outside the main net forwarding lib logic, into the functions that
actually make use of it. My testing system doesn't have mausezahn, and
this test doesn't even require it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../drivers/net/dsa/vlan_validation.sh        | 316 ++++++++++++++++++
 tools/testing/selftests/net/forwarding/lib.sh |   9 +-
 2 files changed, 324 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/dsa/vlan_validation.sh

diff --git a/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
new file mode 100755
index 000000000000..445ce17cb925
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
@@ -0,0 +1,316 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+NUM_NETIFS=2
+lib_dir=$(dirname $0)/../../../net/forwarding
+source $lib_dir/lib.sh
+
+eth0=${NETIFS[p1]}
+eth1=${NETIFS[p2]}
+
+test_bridge_vlan_when_port_has_that_vlan_as_upper()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link set ${eth0} master br0
+	bridge vlan add dev ${eth0} vid 100 master
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "Add bridge VLAN when port has that VLAN as upper already"
+}
+
+test_bridge_vlan_when_port_has_that_vlan_as_upper_but_is_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link set ${eth0} master br0
+	bridge vlan add dev ${eth0} vid 100 master
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100
+	ip link del br0
+
+	log_test "Add bridge VLAN when port has that VLAN as upper already, but bridge is initially VLAN-unaware"
+}
+
+test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link set ${eth0} master br0
+	ip link set ${eth1} master br0
+	bridge vlan add dev ${eth0} vid 100 master
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth1}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "Add bridge VLAN when another bridge port has that VLAN as upper already"
+}
+
+test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper_but_is_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link set ${eth0} master br0
+	ip link set ${eth1} master br0
+	bridge vlan add dev ${eth0} vid 100 master
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Add bridge VLAN when another bridge port has that VLAN as upper already, but bridge is initially VLAN-unaware"
+}
+
+test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	ip link set ${eth1} master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Bridge join when new port has VLAN upper with same VID as another port's VLAN upper"
+}
+
+test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	ip link set ${eth1} master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Bridge join when new port has VLAN upper with same VID as another port's VLAN upper, and bridge is initially unaware"
+}
+
+test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.1 type vlan id 1
+	ip link set ${eth0} master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.1
+	ip link del br0
+
+	log_test "Bridge join when new port has VLAN upper equal to the PVID"
+}
+
+test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid_but_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.1 type vlan id 1
+	ip link set ${eth0} master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.1
+	ip link del br0
+
+	log_test "Bridge join when new port has VLAN upper equal to the PVID, but bridge is initially VLAN-unaware"
+}
+
+test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	bridge vlan add dev ${eth0} vid 100 master
+	ip link set ${eth1} master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Bridge join when new port has VLAN upper with same VID as another port's bridge VLAN"
+}
+
+test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	bridge vlan add dev ${eth0} vid 100 master
+	ip link set ${eth1} master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Bridge join when new port has VLAN upper with same VID as another port's bridge VLAN, but bridge is initially unaware"
+}
+
+test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	ip link set ${eth1} master br0
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Add VLAN upper to port in bridge which has another port with same upper VLAN ID"
+}
+
+test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	ip link set ${eth1} master br0
+	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100
+	ip link del ${eth1}.100
+	ip link del br0
+
+	log_test "Add VLAN upper to port in bridge which has another port with same upper VLAN ID, and bridge is initially unaware"
+}
+
+test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	ip link set ${eth0}.100 master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "VLAN upper joins VLAN-aware bridge"
+}
+
+test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0} master br0
+	ip link set ${eth0}.100 master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "VLAN upper joins VLAN-aware bridge, but bridge is initially unaware"
+}
+
+test_bridge_join_when_vlan_upper_is_already_in_bridge()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0}.100 master br0
+	ip link set ${eth0} master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "Bridge join when VLAN upper is already in VLAN-aware bridge"
+}
+
+test_bridge_join_when_vlan_upper_is_already_in_bridge_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0}.100 master br0
+	ip link set ${eth0} master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "Bridge join when VLAN upper is already in VLAN-aware bridge, which was initially VLAN-unaware"
+}
+
+test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth1} master br0
+	ip link set ${eth0}.100 master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port"
+}
+
+test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth1} master br0
+	ip link set ${eth0}.100 master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port, but bridge is initially unaware"
+}
+
+test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge()
+{
+	ip link add br0 type bridge vlan_filtering 1
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0}.100 master br0
+	ip link set ${eth1} master br0
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "Bridge join when VLAN upper of another port is already in VLAN-aware bridge"
+}
+
+test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware()
+{
+	ip link add br0 type bridge vlan_filtering 0
+	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
+	ip link set ${eth0}.100 master br0
+	ip link set ${eth0} master br0
+	ip link set br0 type bridge vlan_filtering 1
+	check_fail $? "Expected to fail but didn't"
+	ip link del ${eth0}.100 > /dev/null 2>&1 || :
+	ip link del br0
+
+	log_test "Bridge join when VLAN upper of another port is already in VLAN-aware bridge, which was initially VLAN-unaware"
+}
+
+ALL_TESTS="
+	test_bridge_vlan_when_port_has_that_vlan_as_upper
+	test_bridge_vlan_when_port_has_that_vlan_as_upper_but_is_initially_unaware
+	test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper
+	test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper_but_is_initially_unaware
+	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper
+	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper_initially_unaware
+	test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid
+	test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid_but_initially_unaware
+	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan
+	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan_initially_unaware
+	test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid
+	test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid_initially_unaware
+	test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port
+	test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port_initially_unaware
+	test_bridge_join_when_vlan_upper_is_already_in_bridge
+	test_bridge_join_when_vlan_upper_is_already_in_bridge_initially_unaware
+	test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port
+	test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware
+	test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge
+	test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware
+"
+
+tests_run
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index be71012b8fc5..8d7348a1834f 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -139,7 +139,6 @@ require_command()
 }
 
 require_command jq
-require_command $MZ
 
 if [[ ! -v NUM_NETIFS ]]; then
 	echo "SKIP: importer does not define \"NUM_NETIFS\""
@@ -1113,6 +1112,8 @@ learning_test()
 	local mac=de:ad:be:ef:13:37
 	local ageing_time
 
+	require_command $MZ
+
 	RET=0
 
 	bridge -j fdb show br $bridge brport $br_port1 \
@@ -1188,6 +1189,8 @@ flood_test_do()
 	local host2_if=$5
 	local err=0
 
+	require_command $MZ
+
 	# Add an ACL on `host2_if` which will tell us whether the packet
 	# was flooded to it or not.
 	tc qdisc add dev $host2_if ingress
@@ -1276,6 +1279,8 @@ __start_traffic()
 	local dip=$1; shift
 	local dmac=$1; shift
 
+	require_command $MZ
+
 	$MZ $h_in -p 8000 -A $sip -B $dip -c 0 \
 		-a own -b $dmac -t "$proto" -q "$@" &
 	sleep 1
@@ -1352,6 +1357,8 @@ mcast_packet_test()
 	local tc_proto="ip"
 	local mz_v6arg=""
 
+	require_command $MZ
+
 	# basic check to see if we were passed an IPv4 address, if not assume IPv6
 	if [[ ! $ip =~ ^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$ ]]; then
 		tc_proto="ipv6"
-----------------------------[ cut here ]-----------------------------

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Andrew Lunn]

On Tue, Mar 09, 2021 at 10:28:11PM +0100, Tobias Waldekranz wrote:

Hi Tobias

As with Florian, i've not been following the discussion.

> Yeah sorry, I can imagine that makes no sense whatsoever without the
> context of the recent discussions. It is basically guarding against this
> situation:
> 
> .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

Given the complexity at hand, maybe consider putting this all into the
code?

	Andrew

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Andrew Lunn]

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

> > 
> > .100  br0
> >    \  / \
> >    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 set dev lan0 master br0
> > $ ip link set dev lan1 master br0
> > $ bridge vlan add dev lan1 vid 100 # This should fail
> 
> diff --git a/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh

Hi Vladimir

Cool to see self tests.

> new file mode 100755
> index 000000000000..445ce17cb925
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
> @@ -0,0 +1,316 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +NUM_NETIFS=2
> +lib_dir=$(dirname $0)/../../../net/forwarding
> +source $lib_dir/lib.sh
> +
> +eth0=${NETIFS[p1]}
> +eth1=${NETIFS[p2]}

Could these be called lan0 and lan1, so they match the diagrams?  I
find eth0 confusing, since that is often the master interface, not a
slave interface.

      Andrew

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Vladimir Oltean]

On Wed, Mar 10, 2021 at 01:30:33AM +0100, Andrew Lunn wrote:
> On Tue, Mar 09, 2021 at 10:28:11PM +0100, Tobias Waldekranz wrote:
> 
> Hi Tobias
> 
> As with Florian, i've not been following the discussion.

I'm afraid there isn't much context for these patches, except Tobias
pointing out that some of the command sequences I've since spelled out
in the selftest produce results that should be refused but aren't.

But this discussion has actually branched off and into the weeds from a
completely different problem. Tobias reported that DSA attempts to
install VLANs for 8021q uppers into the VTU when the mv88e6xxx ports are
standalone, and the driver doesn't like that. This isn't due to an
invalid constellation, but instead due to improper management of a valid
one.

I've attempted to solve those issues through these three patches, which
at the moment are not properly grouped together, but I think I will pull
them out and send them separately, and let Tobias finish the central
rejection of invalid VLAN constellations:

https://patchwork.kernel.org/project/netdevbpf/patch/20210308135509.3040286-1-olteanv@gmail.com/
https://patchwork.kernel.org/project/netdevbpf/patch/20210309021657.3639745-4-olteanv@gmail.com/
https://patchwork.kernel.org/project/netdevbpf/patch/20210309021657.3639745-5-olteanv@gmail.com/

It would be nice if you could take a look at those three patches before
I resend them tomorrow, also for Tobias to let me know if they fix the
issue he initially reported.

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Tobias Waldekranz]

On Wed, Mar 10, 2021 at 00:01, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Mar 09, 2021 at 10:28:11PM +0100, Tobias Waldekranz wrote:
>> On Tue, Mar 09, 2021 at 12:40, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> > On 3/9/21 10:42 AM, Tobias Waldekranz wrote:
>> >> 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.
>> >
>> > I suppose that is true, given that a non-VLAN filtering bridge must not
>> > perform ingress VID checking, OK.
>> >
>> >> 
>> >> - For all filtering bridges, no stacked VLAN on any port may be
>> >>   configured on multiple ports.
>> >
>> > You need to qualify multiple ports a bit more here, are you saying
>> > multiple ports that are part of said bridge, or?
>> 
>> Yeah sorry, I can imagine that makes no sense whatsoever without the
>> context of the recent discussions. It is basically guarding against this
>> situation:
>> 
>> .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
>> 
>> >> - For all filtering bridges, no stacked VLAN may be configured in the
>> >>   bridge.
>> >
>> > Being stacked in the bridge does not really compute for me, you mean, no
>> > VLAN upper must be configured on the bridge master device(s)? Why would
>> > that be a problem though?
>> 
>> Again sorry, I relize that this message needs a lot of work. It guards
>> against this scenario:
>> 
>> .100  br0
>>    \  / \
>>    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 set dev lan0 master br0
>> $ ip link set dev lan1 master br0
>> $ bridge vlan add dev lan1 vid 100 # This should fail
>> 
>> >> 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@waldekranz.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/
>> >
>> > I really have not been able to keep up with your discussion, and I am
>> > not sure if I will given how quickly you guys can spin patches (not a
>> > criticism, this is welcome).
>> 
>> Yeah I know, it has been a bit of a whirlwind.
>> 
>> Maybe I should just have posted this inline in the other thread, since
>> it was mostly to show Vladimir my idea, and it seemed easier to write it
>> in C than in English :)
>
> I like it, I think it has good potential.
> I wrote up this battery of tests, there is still one condition which you
> are not catching, but you should be able to add it. If you find more

Thanks for taking the time to write all these!

> corner cases please feel free to add them to this list. Then you can
> clean up the patch and send it, I think.
>
> -----------------------------[ cut here ]-----------------------------
> From 9fcfccb6a38a9769962b098ba19d50e576710b5b Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Tue, 9 Mar 2021 23:51:01 +0200
> Subject: [PATCH] selftests: net: dsa: add checks for all VLAN configurations
>  known to mankind that should fail
>
> Offloading VLANs from two different directions is no easy feat,
> especially since we can toggle the VLAN filtering property at runtime,
> and even per port!
>
> Try to capture the combinations of commands that should be rejected by
> DSA, in the attempt of creating a validation procedure that catches them
> all.
>
> Note that this patch moves the irritating "require_command" for mausezahn
> outside the main net forwarding lib logic, into the functions that
> actually make use of it. My testing system doesn't have mausezahn, and
> this test doesn't even require it.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  .../drivers/net/dsa/vlan_validation.sh        | 316 ++++++++++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh |   9 +-
>  2 files changed, 324 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
>
> diff --git a/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
> new file mode 100755
> index 000000000000..445ce17cb925
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/dsa/vlan_validation.sh
> @@ -0,0 +1,316 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +NUM_NETIFS=2
> +lib_dir=$(dirname $0)/../../../net/forwarding
> +source $lib_dir/lib.sh
> +
> +eth0=${NETIFS[p1]}
> +eth1=${NETIFS[p2]}
> +
> +test_bridge_vlan_when_port_has_that_vlan_as_upper()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link set ${eth0} master br0
> +	bridge vlan add dev ${eth0} vid 100 master
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "Add bridge VLAN when port has that VLAN as upper already"
> +}
> +
> +test_bridge_vlan_when_port_has_that_vlan_as_upper_but_is_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link set ${eth0} master br0
> +	bridge vlan add dev ${eth0} vid 100 master
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100
> +	ip link del br0
> +
> +	log_test "Add bridge VLAN when port has that VLAN as upper already, but bridge is initially VLAN-unaware"
> +}
> +
> +test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link set ${eth0} master br0
> +	ip link set ${eth1} master br0
> +	bridge vlan add dev ${eth0} vid 100 master
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth1}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "Add bridge VLAN when another bridge port has that VLAN as upper already"
> +}
> +
> +test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper_but_is_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link set ${eth0} master br0
> +	ip link set ${eth1} master br0
> +	bridge vlan add dev ${eth0} vid 100 master
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Add bridge VLAN when another bridge port has that VLAN as upper already, but bridge is initially VLAN-unaware"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	ip link set ${eth1} master br0
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Bridge join when new port has VLAN upper with same VID as another port's VLAN upper"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	ip link set ${eth1} master br0
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Bridge join when new port has VLAN upper with same VID as another port's VLAN upper, and bridge is initially unaware"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.1 type vlan id 1
> +	ip link set ${eth0} master br0
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.1
> +	ip link del br0
> +
> +	log_test "Bridge join when new port has VLAN upper equal to the PVID"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid_but_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.1 type vlan id 1
> +	ip link set ${eth0} master br0
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.1
> +	ip link del br0
> +
> +	log_test "Bridge join when new port has VLAN upper equal to the PVID, but bridge is initially VLAN-unaware"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	bridge vlan add dev ${eth0} vid 100 master
> +	ip link set ${eth1} master br0
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Bridge join when new port has VLAN upper with same VID as another port's bridge VLAN"
> +}
> +
> +test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	bridge vlan add dev ${eth0} vid 100 master
> +	ip link set ${eth1} master br0
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Bridge join when new port has VLAN upper with same VID as another port's bridge VLAN, but bridge is initially unaware"
> +}
> +
> +test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	ip link set ${eth1} master br0
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Add VLAN upper to port in bridge which has another port with same upper VLAN ID"
> +}
> +
> +test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	ip link set ${eth1} master br0
> +	ip link add link ${eth1} name ${eth1}.100 type vlan id 100
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100
> +	ip link del ${eth1}.100
> +	ip link del br0
> +
> +	log_test "Add VLAN upper to port in bridge which has another port with same upper VLAN ID, and bridge is initially unaware"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	ip link set ${eth0}.100 master br0
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "VLAN upper joins VLAN-aware bridge"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0} master br0
> +	ip link set ${eth0}.100 master br0
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "VLAN upper joins VLAN-aware bridge, but bridge is initially unaware"
> +}
> +
> +test_bridge_join_when_vlan_upper_is_already_in_bridge()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0}.100 master br0
> +	ip link set ${eth0} master br0
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "Bridge join when VLAN upper is already in VLAN-aware bridge"
> +}
> +
> +test_bridge_join_when_vlan_upper_is_already_in_bridge_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0}.100 master br0
> +	ip link set ${eth0} master br0
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "Bridge join when VLAN upper is already in VLAN-aware bridge, which was initially VLAN-unaware"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth1} master br0
> +	ip link set ${eth0}.100 master br0
> +	check_fail $? "Expected to fail but didn't"

Should it though?

   br0
   / \
.100  \
  |    \
eth0   eth1

eth0 is in standalone mode here. So if the kernel allows it, who are we
to argue?

> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port"
> +}
> +
> +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth1} master br0
> +	ip link set ${eth0}.100 master br0
> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"

Same thing here.

> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port, but bridge is initially unaware"
> +}
> +
> +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge()
> +{
> +	ip link add br0 type bridge vlan_filtering 1
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0}.100 master br0
> +	ip link set ${eth1} master br0
> +	check_fail $? "Expected to fail but didn't"

And here.

> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "Bridge join when VLAN upper of another port is already in VLAN-aware bridge"
> +}
> +
> +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware()
> +{
> +	ip link add br0 type bridge vlan_filtering 0
> +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> +	ip link set ${eth0}.100 master br0
> +	ip link set ${eth0} master br0

I think you meant for this to be eth1, correct?

In that case, same comment as before applies to this as well.

> +	ip link set br0 type bridge vlan_filtering 1
> +	check_fail $? "Expected to fail but didn't"
> +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> +	ip link del br0
> +
> +	log_test "Bridge join when VLAN upper of another port is already in VLAN-aware bridge, which was initially VLAN-unaware"
> +}
> +
> +ALL_TESTS="
> +	test_bridge_vlan_when_port_has_that_vlan_as_upper
> +	test_bridge_vlan_when_port_has_that_vlan_as_upper_but_is_initially_unaware
> +	test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper
> +	test_bridge_vlan_when_other_bridge_port_has_that_vlan_as_upper_but_is_initially_unaware
> +	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper
> +	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_vlan_upper_initially_unaware
> +	test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid
> +	test_bridge_join_when_new_port_has_vlan_upper_equal_to_pvid_but_initially_unaware
> +	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan
> +	test_bridge_join_when_new_port_has_vlan_upper_with_same_vid_as_another_port_bridge_vlan_initially_unaware
> +	test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid
> +	test_vlan_upper_on_bridge_port_when_another_port_has_upper_with_same_vid_initially_unaware
> +	test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port
> +	test_vlan_upper_join_vlan_aware_bridge_which_contains_the_physical_port_initially_unaware
> +	test_bridge_join_when_vlan_upper_is_already_in_bridge
> +	test_bridge_join_when_vlan_upper_is_already_in_bridge_initially_unaware
> +	test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port
> +	test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware
> +	test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge
> +	test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware
> +"
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index be71012b8fc5..8d7348a1834f 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -139,7 +139,6 @@ require_command()
>  }
>  
>  require_command jq
> -require_command $MZ
>  
>  if [[ ! -v NUM_NETIFS ]]; then
>  	echo "SKIP: importer does not define \"NUM_NETIFS\""
> @@ -1113,6 +1112,8 @@ learning_test()
>  	local mac=de:ad:be:ef:13:37
>  	local ageing_time
>  
> +	require_command $MZ
> +
>  	RET=0
>  
>  	bridge -j fdb show br $bridge brport $br_port1 \
> @@ -1188,6 +1189,8 @@ flood_test_do()
>  	local host2_if=$5
>  	local err=0
>  
> +	require_command $MZ
> +
>  	# Add an ACL on `host2_if` which will tell us whether the packet
>  	# was flooded to it or not.
>  	tc qdisc add dev $host2_if ingress
> @@ -1276,6 +1279,8 @@ __start_traffic()
>  	local dip=$1; shift
>  	local dmac=$1; shift
>  
> +	require_command $MZ
> +
>  	$MZ $h_in -p 8000 -A $sip -B $dip -c 0 \
>  		-a own -b $dmac -t "$proto" -q "$@" &
>  	sleep 1
> @@ -1352,6 +1357,8 @@ mcast_packet_test()
>  	local tc_proto="ip"
>  	local mz_v6arg=""
>  
> +	require_command $MZ
> +
>  	# basic check to see if we were passed an IPv4 address, if not assume IPv6
>  	if [[ ! $ip =~ ^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$ ]]; then
>  		tc_proto="ipv6"
> -----------------------------[ cut here ]-----------------------------

Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

[Vladimir Oltean]

On Sun, Mar 14, 2021 at 10:40:55PM +0100, Tobias Waldekranz wrote:
> On Wed, Mar 10, 2021 at 00:01, Vladimir Oltean <olteanv@gmail.com> wrote:
> > +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 1
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth1} master br0
> > +	ip link set ${eth0}.100 master br0
> > +	check_fail $? "Expected to fail but didn't"
> 
> Should it though?
> 
>    br0
>    / \
> .100  \
>   |    \
> eth0   eth1
> 
> eth0 is in standalone mode here. So if the kernel allows it, who are we
> to argue?

Without my "call_switchdev_notifiers(SWITCHDEV_BRPORT_OFFLOADED)" patch,
We have the same old problem with bridging with non-offloaded uppers and
the bridge not knowing they aren't offloaded, don't we? The bridge port
will have a wrong offloading mark.

I think in principle the configuration could be supported with software
bridging, and then the dsa_prevent_bridging_8021q_upper restriction can
be lifted, but I imagine we need to add logic for a DSA port offloading
and unoffloading an existing bridge port depending on its upper configuration.

For example, would we support this configuration?

       br0
       /  \
      /    \      br1
     /  eth1.100 /  \
    /       |   /    \
   /        |  /      \
  eth0     eth1      eth2

eth1 would not be "standalone" except from the perspective of br0, but
due to offloading br1, we would need to turn on address learning and
such. So we should probably either enforce that eth1 is standalone when
at least one non-LAG upper is bridged, or deny bridging the non-LAG
uppers. Without a known use case for such configurations, I would rather
deny them for the time being.

> > +	ip link del br0
> > +
> > +	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port"
> > +}
> > +
> > +test_vlan_upper_join_vlan_aware_bridge_which_contains_another_physical_port_initially_unaware()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 0
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth1} master br0
> > +	ip link set ${eth0}.100 master br0
> > +	ip link set br0 type bridge vlan_filtering 1
> > +	check_fail $? "Expected to fail but didn't"
> 
> Same thing here.
> 
> > +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> > +	ip link del br0
> > +
> > +	log_test "VLAN upper joins VLAN-aware bridge which contains another physical port, but bridge is initially unaware"
> > +}
> > +
> > +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 1
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth0}.100 master br0
> > +	ip link set ${eth1} master br0
> > +	check_fail $? "Expected to fail but didn't"
> 
> And here.
> 
> > +	ip link del ${eth0}.100 > /dev/null 2>&1 || :
> > +	ip link del br0
> > +
> > +	log_test "Bridge join when VLAN upper of another port is already in VLAN-aware bridge"
> > +}
> > +
> > +test_bridge_join_when_vlan_upper_of_another_port_is_already_in_bridge_initially_unaware()
> > +{
> > +	ip link add br0 type bridge vlan_filtering 0
> > +	ip link add link ${eth0} name ${eth0}.100 type vlan id 100
> > +	ip link set ${eth0}.100 master br0
> > +	ip link set ${eth0} master br0
> 
> I think you meant for this to be eth1, correct?

Yes, this is a copy-paste mistake.