Date:   Tue, 9 Mar 2021 12:40:08 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>, davem@...emloft.net,
        kuba@...nel.org
Cc:     andrew@...n.ch, vivien.didelot@...il.com, olteanv@...il.com,
        netdev@...r.kernel.org
Subject: Re: [RFC net] net: dsa: Centralize validation of VLAN configuration

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

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

Powered by blists - more mailing lists