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 <tob...@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....@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-olte...@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
