Date:   Tue, 16 Mar 2021 01:49:56 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: Centralize validation of VLAN configuration

On Mon, Mar 15, 2021 at 08:54:13PM +0100, Tobias Waldekranz wrote:
> There are four kinds of events that have an inpact on VLAN

impact

> configuration of DSA ports:
> 
> - Adding VLAN uppers
>   (ip link add dev swp0.1 link swp0 type vlan id 1)
(..)
> +static bool dsa_8021q_uppers_are_coherent(struct dsa_switch_tree *dst,
> +					  struct net_device *br,
> +					  bool seen_vlan_upper,

have_8021q_uppers_in_bridge maybe?

> +					  unsigned long *upper_vids,
> +					  struct netlink_ext_ack *extack)
> +{
> +	struct net_device *lower, *upper;
> +	struct list_head *liter, *uiter;

It doesn't hurt to name them lower_iter, upper_iter?

> +	struct dsa_slave_priv *priv;
> +	bool seen_offloaded = false;
> +	u16 vid;
> +
> +	netdev_for_each_lower_dev(br, lower, liter) {
> +		priv = dsa_slave_dev_lower_find(lower);
> +		if (!priv || priv->dp->ds->dst != dst)
> +			/* Ignore ports that are not related to us in
> +			 * any way.
> +			 */
> +			continue;

So "priv" is the lower of a bridge port...

> +
> +		if (is_vlan_dev(lower)) {
> +			seen_vlan_upper = true;
> +			continue;
> +		}

But in the code path below, that bridge port is not a VLAN... So it must
be a LAG or a HSR ring....

> +		if (dsa_port_offloads_bridge(priv->dp, br) &&
> +		    dsa_port_offloads_bridge_port(priv->dp, lower))
> +			seen_offloaded = true;
> +		else
> +			/* Non-offloaded uppers can to whatever they

s/can to/can do/

> +			 * want.
> +			 */
> +			continue;

Which is offloaded..

> +		netdev_for_each_upper_dev_rcu(lower, upper, uiter) {
> +			if (!is_vlan_dev(upper))
> +				continue;

So this iterates through VLAN uppers of offloaded LAGs and HSR rings?
Does it also iterate through 8021q uppers of "priv" somehow?

> +			vid = vlan_dev_vlan_id(upper);
> +			if (!test_bit(vid, upper_vids)) {
> +				set_bit(vid, upper_vids);
> +				continue;
> +			}
> +
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Multiple VLAN interfaces cannot use the same VID");
> +			return false;
> +		}
> +	}
> +
> +	if (seen_offloaded && seen_vlan_upper) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "VLAN interfaces cannot share bridge with offloaded port");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool dsa_bridge_vlans_are_coherent(struct net_device *br,
> +					  u16 new_vid, unsigned long *upper_vids,

const unsigned long *upper_vids

> +					  struct netlink_ext_ack *extack)
> +{
> +	u16 vid;
> +
> +	if (new_vid && test_bit(new_vid, upper_vids))
> +		goto err;
> +
> +	for_each_set_bit(vid, upper_vids, VLAN_N_VID) {
> +		struct bridge_vlan_info br_info;
> +
> +		if (br_vlan_get_info(br, vid, &br_info))

You should only error out if VLAN filtering is enabled/turning on in the
bridge, no?

> +			/* Error means that the VID does not exist,
> +			 * which is what we want to ensure.
> +			 */
> +			continue;
> +
> +		goto err;
> +	}
> +
> +	return true;
> +
> +err:
> +	NL_SET_ERR_MSG_MOD(extack, "No bridge VID may be used on a related VLAN interface");
> +	return false;
> +}
> +
> +/**
> + * dsa_bridge_is_coherent - Verify that DSA tree accepts a bridge config.
> + * @dst: Tree to verify against.
> + * @br: Bridge netdev to verify.
> + * @mod: Description of the modification to introduce.
> + * @extack: Netlink extended ack for error reporting.
> + *
> + * Verify that the VLAN config of @br, its offloaded ports belonging
> + * to @dst and their VLAN uppers, can be correctly offloaded after
> + * introducing the change described by @mod. If this is not the case,
> + * an error is reported via @extack.
> + *
> + * Return: true if the config can be offloaded, false otherwise.
> + */
> +bool dsa_bridge_is_coherent(struct dsa_switch_tree *dst, struct net_device *br,
> +			    struct dsa_bridge_mod *mod,
> +			    struct netlink_ext_ack *extack)
> +{
> +	unsigned long *upper_vids = NULL;

initialization with NULL is pointless.

> +	bool filter;
> +
> +	if (mod->filter)
> +		filter = *mod->filter;
> +	else
> +		filter = br && br_vlan_enabled(br);
> +
> +	if (!dsa_bridge_filtering_is_coherent(dst, filter, extack))
> +		goto err;
> +
> +	if (!filter)
> +		return true;
> +
> +	upper_vids = bitmap_zalloc(VLAN_N_VID, GFP_KERNEL);
> +	if (!upper_vids) {
> +		WARN_ON_ONCE(1);

if (WARN_ON_ONCE(!upper_vids))

> +		goto err;
> +	}
> +
> +	if (mod->upper_vid)
> +		set_bit(mod->upper_vid, upper_vids);
> +
> +	if (!dsa_8021q_uppers_are_coherent(dst, br, mod->bridge_upper,
> +					   upper_vids, extack))
> +		goto err_free;
> +
> +	if (!dsa_bridge_vlans_are_coherent(br, mod->br_vid, upper_vids, extack))
> +		goto err_free;
> +
> +	kfree(upper_vids);
> +	return true;
> +
> +err_free:
> +	kfree(upper_vids);
> +err:
> +	return false;
> +}
> +
>  /**
>   * dsa_tree_notify - Execute code for all switches in a DSA switch tree.
>   * @dst: collection of struct dsa_switch devices to notify.
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 9d4b0e9b1aa1..8d8d307df437 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -361,6 +369,27 @@ int dsa_switch_register_notifier(struct dsa_switch *ds);
>  void dsa_switch_unregister_notifier(struct dsa_switch *ds);
>  
>  /* dsa2.c */
> +
> +/**
> + * struct dsa_bridge_mod - Modification of bridge related config
> + * @filter: If non-NULL, the new state of VLAN filtering.
> + * @br_vid: If non-zero, this VID will be added to the bridge.
> + * @upper_vid: If non-zero, a VLAN upper using this VID will be added to
> + *             a bridge port.
> + * @bridge_upper: If non-NULL, a VLAN upper will be added to the bridge.

I would name this "add_8021q_upper_to_bridge". Longer name, but clearer.

> + *
> + * Describes a bridge related modification that is about to be applied.
> + */
> +struct dsa_bridge_mod {
> +	bool *filter;
> +	u16   br_vid;
> +	u16   upper_vid;
> +	bool  bridge_upper;
> +};

Frankly this is a bit ugly, but I have no better idea, and the structure
is good enough for describing a state transition. Fully describing the
state is a lot more difficult, due to the need to list all bridges which
may span a DSA switch tree.

> +bool dsa_bridge_is_coherent(struct dsa_switch_tree *dst, struct net_device *br,
> +			    struct dsa_bridge_mod *mod,
> +			    struct netlink_ext_ack *extack);
>  void dsa_lag_map(struct dsa_switch_tree *dst, struct net_device *lag);
>  void dsa_lag_unmap(struct dsa_switch_tree *dst, struct net_device *lag);
>  int dsa_tree_notify(struct dsa_switch_tree *dst, unsigned long e, void *v);
(...)
> -static struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
> +struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
>  {
>  	struct netdev_nested_priv priv = {
>  		.data = NULL,
>  	};
>  
> +	if (dsa_slave_dev_check(dev))
> +		return netdev_priv(dev);
> +
>  	netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv);
>  
>  	return (struct dsa_slave_priv *)priv.data;

Ah, so that's what you did there. I don't like it. If the function is
called "lower_find" and you come back with "dev" itself, I think that
would qualify as "unexpected". Could you create a new function called
dsa_slave_find_in_lowers_or_self, or something like that, which calls
dsa_slave_dev_lower_find with the extra identity check?

Powered by blists - more mailing lists