[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <699042d3-e124-7584-6486-02a6fb45423e@gmail.com>
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