Message-ID: <878s6nozcz.fsf@waldekranz.com>
Date:   Tue, 16 Mar 2021 17:08:28 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.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 Tue, Mar 16, 2021 at 01:49, Vladimir Oltean <olteanv@...il.com> wrote:
> 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)
> (..)

Parse error; I need more context :)

>> +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?

I like that the current name hints of a relation with
seen_offloaded. Your suggestion seems awfully long for an argument name.

>
>> +					  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?

As you discovered below, dsa_slave_dev_lower_find now also matches the
starting device as well as any device below it. So we iterate through
all uppers of any bridge port that this tree is offloading.

>> +			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?

We only validate upper and bridge VLAN coherency for filtering
bridges. Otherwise we return early from dsa_bridge_is_coherent.

>> +			/* 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.

It is not like it is a global variable or anything, there is plenty of
context here I think. You know that you are describing a bridge related
VLAN modification.

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

I am not sure what to make of this. Its job _is_ to describe a state
transition. Why would we want to describe the state? The kernel already
has the state, which is what dsa_bridge_is_coherent uses to figure out
if the change can be applied or not.

Is it sexy? No, I guess not. This type of code seldom is. The
alternative would be to cram the info into the argument list, but that
makes the wrappers harder to read and it makes it harder to extend when
we want to validate another invariant.

>> +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?

My assumption was the opposite. Looking through the kernel, there seem
to be three other logical equivalents:

drivers/net/ethernet/marvell/prestera/prestera_main.c
495:struct prestera_port *prestera_port_dev_lower_find(struct net_device *dev)

drivers/net/ethernet/mellanox/mlxsw/spectrum.c
3406:struct mlxsw_sp_port *mlxsw_sp_port_dev_lower_find(struct net_device *dev)

drivers/net/ethernet/rocker/rocker_main.c
3090:struct rocker_port *rocker_port_dev_lower_find(struct net_device *dev,

All three will check the starting device before walking any lowers.

Thank you!

Powered by blists - more mailing lists