Re: [PATCH net] net: dsa: Centralize validation of VLAN configuration
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-03-15 23:51:02
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))
quoted hunk ↗ jump to hunk
+ 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?