* [PATCH net 0/2] net: dsa: Avoid VLAN config corruption
@ 2021-03-06 0:24 Tobias Waldekranz
2021-03-06 0:24 ` [PATCH net 1/2] Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored" Tobias Waldekranz
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Tobias Waldekranz @ 2021-03-06 0:24 UTC (permalink / raw)
To: davem, kuba; +Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev
The story here is basically:
1. Bridge port attributes should not be offloaded if an intermediate
stacked device (a LAG) is not offloaded. (5696c8aedfcc)
2. (1) broke VLAN filtering events from being processed by DSA, we
must accept that orig_dev can be the bridge itself. (99b8202b179f)
3. (2) broke regular old VLAN configuration, as events generated to
notify the ports that a new VLAN was created in the bridge were now
interpreted as that VLAN being added to the port.
Which brings us to this series, which tries to put an end to this saga
by reverting (2) and then provides a new fix for that issue which
accepts that orig_dev may be the bridge master, but only for
applicable attributes, and never for switchdev objects.
I am not really sure about the process here. Is it fine to revert even
if that re-introduces a bug that is then fixed in a followup commit,
or should this be squashed to a single commit?
Tobias Waldekranz (2):
Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting
ignored"
net: dsa: Always react to global bridge attribute changes
net/dsa/dsa_priv.h | 10 +---------
net/dsa/slave.c | 17 +++++++++++++++--
2 files changed, 16 insertions(+), 11 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH net 1/2] Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored" 2021-03-06 0:24 [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Tobias Waldekranz @ 2021-03-06 0:24 ` Tobias Waldekranz 2021-03-06 0:24 ` [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes Tobias Waldekranz 2021-03-06 0:41 ` [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Vladimir Oltean 2 siblings, 0 replies; 10+ messages in thread From: Tobias Waldekranz @ 2021-03-06 0:24 UTC (permalink / raw) To: davem, kuba; +Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev This reverts commit 99b8202b179fc3dbbca69e8af6da660224c9d676. DSA should indeed react to certain switchdev attributes where orig_dev is the bridge master. But the fix was too broad, causing the driver to misinterpret VLAN objects added to the bridge master as objects added to the ports. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- net/dsa/dsa_priv.h | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 2eeaa42f2e08..d40dfede494c 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -236,15 +236,7 @@ static inline bool dsa_port_offloads_netdev(struct dsa_port *dp, /* Switchdev offloading can be configured on: */ if (dev == dp->slave) - /* DSA ports directly connected to a bridge, and event - * was emitted for the ports themselves. - */ - return true; - - if (dp->bridge_dev == dev) - /* DSA ports connected to a bridge, and event was emitted - * for the bridge. - */ + /* DSA ports directly connected to a bridge. */ return true; if (dp->lag_dev == dev) -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-06 0:24 [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Tobias Waldekranz 2021-03-06 0:24 ` [PATCH net 1/2] Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored" Tobias Waldekranz @ 2021-03-06 0:24 ` Tobias Waldekranz 2021-03-06 14:00 ` Vladimir Oltean 2021-03-06 0:41 ` [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Vladimir Oltean 2 siblings, 1 reply; 10+ messages in thread From: Tobias Waldekranz @ 2021-03-06 0:24 UTC (permalink / raw) To: davem, kuba; +Cc: andrew, vivien.didelot, f.fainelli, olteanv, netdev This is the second attempt to provide a fix for the issue described in 99b8202b179f, which was reverted in the previous commit. When a change is made to some global bridge attribute, such as VLAN filtering, accept events where orig_dev is the bridge master netdev. Separate the validation of orig_dev based on whether the attribute in question is global or per-port. Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- net/dsa/slave.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 491e3761b5f4..63ee2cae4d8e 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -278,8 +278,21 @@ static int dsa_slave_port_attr_set(struct net_device *dev, struct dsa_port *dp = dsa_slave_to_port(dev); int ret; - if (!dsa_port_offloads_netdev(dp, attr->orig_dev)) - return -EOPNOTSUPP; + switch (attr->id) { + case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING: + case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME: + case SWITCHDEV_ATTR_ID_BRIDGE_MROUTER: + /* For global bridge settings, the originating device + * may be the bridge itself. + */ + if (netif_is_bridge_master(attr->orig_dev)) + break; + + fallthrough; + default: + if (!dsa_port_offloads_netdev(dp, attr->orig_dev)) + return -EOPNOTSUPP; + } switch (attr->id) { case SWITCHDEV_ATTR_ID_PORT_STP_STATE: -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-06 0:24 ` [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes Tobias Waldekranz @ 2021-03-06 14:00 ` Vladimir Oltean 2021-03-06 14:04 ` Vladimir Oltean 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2021-03-06 14:00 UTC (permalink / raw) To: Tobias Waldekranz; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev Hi Tobias, On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote: > This is the second attempt to provide a fix for the issue described in > 99b8202b179f, which was reverted in the previous commit. > > When a change is made to some global bridge attribute, such as VLAN > filtering, accept events where orig_dev is the bridge master netdev. > > Separate the validation of orig_dev based on whether the attribute in > question is global or per-port. > > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- What do you think about this alternative? -----------------------------[ cut here ]----------------------------- From af528ac6de2b16df4c8ba21bc7d978984883f319 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: Sat, 6 Mar 2021 15:47:01 +0200 Subject: [PATCH] net: dsa: fix switchdev objects on bridge master mistakenly being applied on ports Tobias reports that the blamed patch was too broad, and now, VLAN objects being added to a bridge: ip link add br0 type bridge vlan_filtering 1 ip link set swp2 master br0 ip link set swp3 master br0 bridge vlan add dev br0 vid 100 self are being added to all slave ports instead (swp2, swp3). This is because the fix was too broad: we made dsa_port_offloads_netdev say "yes, I offload the br0 bridge" for all slave ports, but we didn't add the checks whether the switchdev object was in fact meant for the physical port or for the bridge itself. Let's keep that definition of dsa_port_offloads_netdev, and just add the checks for the individual switchdev object types and attributes that can be notified both on a physical port as well as on the bridge network interface. This will allow us in the future to do things such as offload VLANs on the bridge interface by sending them to the CPU port, instead of the crude "all VLANs on user ports must be added to the CPU port too" logic that we have now. It will also allow us to properly support the drivers that don't implement .port_bridge_add and .port_bridge_leave, because we'll still have an accurate answer to the question "dsa_port_offloads_netdev". Fixes: 99b8202b179f ("net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored") Reported-by: Tobias Waldekranz <tobias@waldekranz.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/dsa/slave.c | 60 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 18 deletions(-) diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 491e3761b5f4..6e6384b2d5d2 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -283,6 +283,9 @@ static int dsa_slave_port_attr_set(struct net_device *dev, switch (attr->id) { case SWITCHDEV_ATTR_ID_PORT_STP_STATE: + if (!dsa_slave_dev_check(dev)) + return 0; + ret = dsa_port_set_state(dp, attr->u.stp_state); break; case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING: @@ -290,13 +293,22 @@ static int dsa_slave_port_attr_set(struct net_device *dev, extack); break; case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME: + if (!dsa_slave_dev_check(attr->orig_dev)) + return 0; + ret = dsa_port_ageing_time(dp, attr->u.ageing_time); break; case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS: + if (!dsa_slave_dev_check(attr->orig_dev)) + return 0; + ret = dsa_port_pre_bridge_flags(dp, attr->u.brport_flags, extack); break; case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS: + if (!dsa_slave_dev_check(attr->orig_dev)) + return 0; + ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, extack); break; case SWITCHDEV_ATTR_ID_BRIDGE_MROUTER: @@ -341,9 +353,6 @@ static int dsa_slave_vlan_add(struct net_device *dev, struct switchdev_obj_port_vlan vlan; int err; - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) { NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN"); return 0; @@ -389,10 +398,14 @@ static int dsa_slave_port_obj_add(struct net_device *dev, struct dsa_port *dp = dsa_slave_to_port(dev); int err; + if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + return -EOPNOTSUPP; + switch (obj->id) { case SWITCHDEV_OBJ_ID_PORT_MDB: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_HOST_MDB: @@ -402,16 +415,21 @@ static int dsa_slave_port_obj_add(struct net_device *dev, err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_PORT_VLAN: + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_slave_vlan_add(dev, obj, extack); break; case SWITCHDEV_OBJ_ID_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_port_mrp_add(dp, SWITCHDEV_OBJ_MRP(obj)); break; case SWITCHDEV_OBJ_ID_RING_ROLE_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_port_mrp_add_ring_role(dp, SWITCHDEV_OBJ_RING_ROLE_MRP(obj)); break; @@ -431,9 +449,6 @@ static int dsa_slave_vlan_del(struct net_device *dev, struct switchdev_obj_port_vlan *vlan; int err; - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) return 0; @@ -457,10 +472,14 @@ static int dsa_slave_port_obj_del(struct net_device *dev, struct dsa_port *dp = dsa_slave_to_port(dev); int err; + if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + return -EOPNOTSUPP; + switch (obj->id) { case SWITCHDEV_OBJ_ID_PORT_MDB: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_HOST_MDB: @@ -470,16 +489,21 @@ static int dsa_slave_port_obj_del(struct net_device *dev, err = dsa_port_mdb_del(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_PORT_VLAN: + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_slave_vlan_del(dev, obj); break; case SWITCHDEV_OBJ_ID_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_port_mrp_del(dp, SWITCHDEV_OBJ_MRP(obj)); break; case SWITCHDEV_OBJ_ID_RING_ROLE_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (!dsa_slave_dev_check(obj->orig_dev)) + return 0; + err = dsa_port_mrp_del_ring_role(dp, SWITCHDEV_OBJ_RING_ROLE_MRP(obj)); break; -----------------------------[ cut here ]----------------------------- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-06 14:00 ` Vladimir Oltean @ 2021-03-06 14:04 ` Vladimir Oltean 2021-03-06 18:17 ` Tobias Waldekranz 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2021-03-06 14:04 UTC (permalink / raw) To: Tobias Waldekranz; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote: > Hi Tobias, > > On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote: > > This is the second attempt to provide a fix for the issue described in > > 99b8202b179f, which was reverted in the previous commit. > > > > When a change is made to some global bridge attribute, such as VLAN > > filtering, accept events where orig_dev is the bridge master netdev. > > > > Separate the validation of orig_dev based on whether the attribute in > > question is global or per-port. > > > > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > > --- > > What do you think about this alternative? Ah, wait, this won't work when offloading objects/attributes on a LAG. Let me actually test your patch. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-06 14:04 ` Vladimir Oltean @ 2021-03-06 18:17 ` Tobias Waldekranz 2021-03-07 0:58 ` Vladimir Oltean 0 siblings, 1 reply; 10+ messages in thread From: Tobias Waldekranz @ 2021-03-06 18:17 UTC (permalink / raw) To: Vladimir Oltean; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean <olteanv@gmail.com> wrote: > On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote: >> Hi Tobias, >> >> On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote: >> > This is the second attempt to provide a fix for the issue described in >> > 99b8202b179f, which was reverted in the previous commit. >> > >> > When a change is made to some global bridge attribute, such as VLAN >> > filtering, accept events where orig_dev is the bridge master netdev. >> > >> > Separate the validation of orig_dev based on whether the attribute in >> > question is global or per-port. >> > >> > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") >> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> > --- >> >> What do you think about this alternative? > > Ah, wait, this won't work when offloading objects/attributes on a LAG. > Let me actually test your patch. Right. But you made me realize that my v1 is also flawed, because it does not guard against trying to apply attributes to non-offloaded ports. ...the original issue :facepalm: I have a version ready which reuses the exact predicate that you previously added to dsa_port_offloads_netdev: - if (netif_is_bridge_master(attr->orig_dev)) + if (dp->bridge_dev == attr->orig_dev) Do you think anything else needs to be changed, or should I send that as v2? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-06 18:17 ` Tobias Waldekranz @ 2021-03-07 0:58 ` Vladimir Oltean 2021-03-07 9:51 ` Tobias Waldekranz 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2021-03-07 0:58 UTC (permalink / raw) To: Tobias Waldekranz; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev On Sat, Mar 06, 2021 at 07:17:09PM +0100, Tobias Waldekranz wrote: > On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote: > >> Hi Tobias, > >> > >> On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote: > >> > This is the second attempt to provide a fix for the issue described in > >> > 99b8202b179f, which was reverted in the previous commit. > >> > > >> > When a change is made to some global bridge attribute, such as VLAN > >> > filtering, accept events where orig_dev is the bridge master netdev. > >> > > >> > Separate the validation of orig_dev based on whether the attribute in > >> > question is global or per-port. > >> > > >> > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") > >> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > >> > --- > >> > >> What do you think about this alternative? > > > > Ah, wait, this won't work when offloading objects/attributes on a LAG. > > Let me actually test your patch. > > Right. But you made me realize that my v1 is also flawed, because it > does not guard against trying to apply attributes to non-offloaded > ports. ...the original issue :facepalm: > > I have a version ready which reuses the exact predicate that you > previously added to dsa_port_offloads_netdev: > > - if (netif_is_bridge_master(attr->orig_dev)) > + if (dp->bridge_dev == attr->orig_dev) > > Do you think anything else needs to be changed, or should I send that as > v2? Sorry, I just get a blank stare when I look at that blob of code you've added at the beginning of dsa_slave_port_attr_set, it might as well be correct but I'm not smart enough to process it and say "yes it is". What do you think about this one? At least for me it's easier to understand what's going on, and would leave a lot more room for further fixups if needed. -----------------------------[ cut here ]----------------------------- From 1f1d463788ace3434878f16aa2f083db2b007990 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: Sat, 6 Mar 2021 01:24:54 +0100 Subject: [PATCH] net: dsa: fix switchdev objects on bridge master mistakenly being applied on ports Tobias reports that the blamed patch was too broad, and now, VLAN objects being added to a bridge: ip link add br0 type bridge vlan_filtering 1 ip link set swp2 master br0 ip link set swp3 master br0 bridge vlan add dev br0 vid 100 self are being added to all slave ports instead (swp2, swp3). This is because the fix was too broad: we made dsa_port_offloads_netdev say "yes, I offload the br0 bridge" for all slave ports, but we didn't add the checks whether the switchdev object was in fact meant for the physical port or for the bridge itself. The reason why the fix was too broad is because the question itself, "does this DSA port offload this netdev", was too broad in the first place. The solution is to disambiguate the question and separate it into two different functions, one to be called for each switchdev attribute / object that has an orig_dev == net_bridge (dsa_port_offloads_bridge), and the other for orig_dev == net_bridge_port (*_offloads_bridge_port). Fixes: 99b8202b179f ("net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored") Reported-by: Tobias Waldekranz <tobias@waldekranz.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/dsa/dsa_priv.h | 25 +++++++++++--------- net/dsa/slave.c | 59 +++++++++++++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 27 deletions(-) diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 2eeaa42f2e08..9d4b0e9b1aa1 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -230,8 +230,8 @@ int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr); void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr); extern const struct phylink_mac_ops dsa_port_phylink_mac_ops; -static inline bool dsa_port_offloads_netdev(struct dsa_port *dp, - struct net_device *dev) +static inline bool dsa_port_offloads_bridge_port(struct dsa_port *dp, + struct net_device *dev) { /* Switchdev offloading can be configured on: */ @@ -241,12 +241,6 @@ static inline bool dsa_port_offloads_netdev(struct dsa_port *dp, */ return true; - if (dp->bridge_dev == dev) - /* DSA ports connected to a bridge, and event was emitted - * for the bridge. - */ - return true; - if (dp->lag_dev == dev) /* DSA ports connected to a bridge via a LAG */ return true; @@ -254,14 +248,23 @@ static inline bool dsa_port_offloads_netdev(struct dsa_port *dp, return false; } +static inline bool dsa_port_offloads_bridge(struct dsa_port *dp, + struct net_device *bridge_dev) +{ + /* DSA ports connected to a bridge, and event was emitted + * for the bridge. + */ + return dp->bridge_dev == bridge_dev; +} + /* Returns true if any port of this tree offloads the given net_device */ -static inline bool dsa_tree_offloads_netdev(struct dsa_switch_tree *dst, - struct net_device *dev) +static inline bool dsa_tree_offloads_bridge_port(struct dsa_switch_tree *dst, + struct net_device *dev) { struct dsa_port *dp; list_for_each_entry(dp, &dst->ports, list) - if (dsa_port_offloads_netdev(dp, dev)) + if (dsa_port_offloads_bridge_port(dp, dev)) return true; return false; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 491e3761b5f4..992fcab4b552 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -278,28 +278,43 @@ static int dsa_slave_port_attr_set(struct net_device *dev, struct dsa_port *dp = dsa_slave_to_port(dev); int ret; - if (!dsa_port_offloads_netdev(dp, attr->orig_dev)) - return -EOPNOTSUPP; - switch (attr->id) { case SWITCHDEV_ATTR_ID_PORT_STP_STATE: + if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev)) + return -EOPNOTSUPP; + ret = dsa_port_set_state(dp, attr->u.stp_state); break; case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING: + if (!dsa_port_offloads_bridge(dp, attr->orig_dev)) + return -EOPNOTSUPP; + ret = dsa_port_vlan_filtering(dp, attr->u.vlan_filtering, extack); break; case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME: + if (!dsa_port_offloads_bridge(dp, attr->orig_dev)) + return -EOPNOTSUPP; + ret = dsa_port_ageing_time(dp, attr->u.ageing_time); break; case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS: + if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev)) + return -EOPNOTSUPP; + ret = dsa_port_pre_bridge_flags(dp, attr->u.brport_flags, extack); break; case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS: + if (!dsa_port_offloads_bridge_port(dp, attr->orig_dev)) + return -EOPNOTSUPP; + ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, extack); break; case SWITCHDEV_ATTR_ID_BRIDGE_MROUTER: + if (!dsa_port_offloads_bridge(dp, attr->orig_dev)) + return -EOPNOTSUPP; + ret = dsa_port_mrouter(dp->cpu_dp, attr->u.mrouter, extack); break; default: @@ -341,9 +356,6 @@ static int dsa_slave_vlan_add(struct net_device *dev, struct switchdev_obj_port_vlan vlan; int err; - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) { NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN"); return 0; @@ -391,27 +403,36 @@ static int dsa_slave_port_obj_add(struct net_device *dev, switch (obj->id) { case SWITCHDEV_OBJ_ID_PORT_MDB: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) return -EOPNOTSUPP; + err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_HOST_MDB: + if (!dsa_port_offloads_bridge(dp, obj->orig_dev)) + return -EOPNOTSUPP; + /* DSA can directly translate this to a normal MDB add, * but on the CPU port. */ err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_PORT_VLAN: + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) + return -EOPNOTSUPP; + err = dsa_slave_vlan_add(dev, obj, extack); break; case SWITCHDEV_OBJ_ID_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + if (!dsa_port_offloads_bridge(dp, obj->orig_dev)) return -EOPNOTSUPP; + err = dsa_port_mrp_add(dp, SWITCHDEV_OBJ_MRP(obj)); break; case SWITCHDEV_OBJ_ID_RING_ROLE_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + if (!dsa_port_offloads_bridge(dp, obj->orig_dev)) return -EOPNOTSUPP; + err = dsa_port_mrp_add_ring_role(dp, SWITCHDEV_OBJ_RING_ROLE_MRP(obj)); break; @@ -431,9 +452,6 @@ static int dsa_slave_vlan_del(struct net_device *dev, struct switchdev_obj_port_vlan *vlan; int err; - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) - return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) return 0; @@ -459,27 +477,36 @@ static int dsa_slave_port_obj_del(struct net_device *dev, switch (obj->id) { case SWITCHDEV_OBJ_ID_PORT_MDB: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) return -EOPNOTSUPP; + err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_HOST_MDB: + if (!dsa_port_offloads_bridge(dp, obj->orig_dev)) + return -EOPNOTSUPP; + /* DSA can directly translate this to a normal MDB add, * but on the CPU port. */ err = dsa_port_mdb_del(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_PORT_VLAN: + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) + return -EOPNOTSUPP; + err = dsa_slave_vlan_del(dev, obj); break; case SWITCHDEV_OBJ_ID_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + if (!dsa_port_offloads_bridge(dp, obj->orig_dev)) return -EOPNOTSUPP; + err = dsa_port_mrp_del(dp, SWITCHDEV_OBJ_MRP(obj)); break; case SWITCHDEV_OBJ_ID_RING_ROLE_MRP: - if (!dsa_port_offloads_netdev(dp, obj->orig_dev)) + if (!dsa_port_offloads_bridge(dp, obj->orig_dev)) return -EOPNOTSUPP; + err = dsa_port_mrp_del_ring_role(dp, SWITCHDEV_OBJ_RING_ROLE_MRP(obj)); break; @@ -2298,7 +2325,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused, * other ports bridged with the LAG should be able to * autonomously forward towards it. */ - if (dsa_tree_offloads_netdev(dp->ds->dst, dev)) + if (dsa_tree_offloads_bridge_port(dp->ds->dst, dev)) return NOTIFY_DONE; } -----------------------------[ cut here ]----------------------------- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-07 0:58 ` Vladimir Oltean @ 2021-03-07 9:51 ` Tobias Waldekranz 2021-03-07 10:07 ` Vladimir Oltean 0 siblings, 1 reply; 10+ messages in thread From: Tobias Waldekranz @ 2021-03-07 9:51 UTC (permalink / raw) To: Vladimir Oltean; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev On Sun, Mar 07, 2021 at 02:58, Vladimir Oltean <olteanv@gmail.com> wrote: > On Sat, Mar 06, 2021 at 07:17:09PM +0100, Tobias Waldekranz wrote: >> On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean <olteanv@gmail.com> wrote: >> > On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote: >> >> Hi Tobias, >> >> >> >> On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote: >> >> > This is the second attempt to provide a fix for the issue described in >> >> > 99b8202b179f, which was reverted in the previous commit. >> >> > >> >> > When a change is made to some global bridge attribute, such as VLAN >> >> > filtering, accept events where orig_dev is the bridge master netdev. >> >> > >> >> > Separate the validation of orig_dev based on whether the attribute in >> >> > question is global or per-port. >> >> > >> >> > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") >> >> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> >> > --- >> >> >> >> What do you think about this alternative? >> > >> > Ah, wait, this won't work when offloading objects/attributes on a LAG. >> > Let me actually test your patch. >> >> Right. But you made me realize that my v1 is also flawed, because it >> does not guard against trying to apply attributes to non-offloaded >> ports. ...the original issue :facepalm: >> >> I have a version ready which reuses the exact predicate that you >> previously added to dsa_port_offloads_netdev: >> >> - if (netif_is_bridge_master(attr->orig_dev)) >> + if (dp->bridge_dev == attr->orig_dev) >> >> Do you think anything else needs to be changed, or should I send that as >> v2? > > Sorry, I just get a blank stare when I look at that blob of code you've > added at the beginning of dsa_slave_port_attr_set, it might as well be > correct but I'm not smart enough to process it and say "yes it is". > > What do you think about this one? At least for me it's easier to > understand what's going on, and would leave a lot more room for further > fixups if needed. I like the approach of having to explicitly state the supported orig_dev per attribute or object. I think we should go with your fix. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes 2021-03-07 9:51 ` Tobias Waldekranz @ 2021-03-07 10:07 ` Vladimir Oltean 0 siblings, 0 replies; 10+ messages in thread From: Vladimir Oltean @ 2021-03-07 10:07 UTC (permalink / raw) To: Tobias Waldekranz; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev On Sun, Mar 07, 2021 at 10:51:18AM +0100, Tobias Waldekranz wrote: > On Sun, Mar 07, 2021 at 02:58, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Sat, Mar 06, 2021 at 07:17:09PM +0100, Tobias Waldekranz wrote: > >> On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean <olteanv@gmail.com> wrote: > >> > On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote: > >> >> Hi Tobias, > >> >> > >> >> On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote: > >> >> > This is the second attempt to provide a fix for the issue described in > >> >> > 99b8202b179f, which was reverted in the previous commit. > >> >> > > >> >> > When a change is made to some global bridge attribute, such as VLAN > >> >> > filtering, accept events where orig_dev is the bridge master netdev. > >> >> > > >> >> > Separate the validation of orig_dev based on whether the attribute in > >> >> > question is global or per-port. > >> >> > > >> >> > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports") > >> >> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > >> >> > --- > >> >> > >> >> What do you think about this alternative? > >> > > >> > Ah, wait, this won't work when offloading objects/attributes on a LAG. > >> > Let me actually test your patch. > >> > >> Right. But you made me realize that my v1 is also flawed, because it > >> does not guard against trying to apply attributes to non-offloaded > >> ports. ...the original issue :facepalm: > >> > >> I have a version ready which reuses the exact predicate that you > >> previously added to dsa_port_offloads_netdev: > >> > >> - if (netif_is_bridge_master(attr->orig_dev)) > >> + if (dp->bridge_dev == attr->orig_dev) > >> > >> Do you think anything else needs to be changed, or should I send that as > >> v2? > > > > Sorry, I just get a blank stare when I look at that blob of code you've > > added at the beginning of dsa_slave_port_attr_set, it might as well be > > correct but I'm not smart enough to process it and say "yes it is". > > > > What do you think about this one? At least for me it's easier to > > understand what's going on, and would leave a lot more room for further > > fixups if needed. > > I like the approach of having to explicitly state the supported orig_dev > per attribute or object. I think we should go with your fix. Ok, I'm sending it as-is. Thanks again! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 0/2] net: dsa: Avoid VLAN config corruption 2021-03-06 0:24 [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Tobias Waldekranz 2021-03-06 0:24 ` [PATCH net 1/2] Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored" Tobias Waldekranz 2021-03-06 0:24 ` [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes Tobias Waldekranz @ 2021-03-06 0:41 ` Vladimir Oltean 2 siblings, 0 replies; 10+ messages in thread From: Vladimir Oltean @ 2021-03-06 0:41 UTC (permalink / raw) To: Tobias Waldekranz; +Cc: davem, kuba, andrew, vivien.didelot, f.fainelli, netdev On Sat, Mar 06, 2021 at 01:24:53AM +0100, Tobias Waldekranz wrote: > The story here is basically: > > 1. Bridge port attributes should not be offloaded if an intermediate > stacked device (a LAG) is not offloaded. (5696c8aedfcc) > > 2. (1) broke VLAN filtering events from being processed by DSA, we > must accept that orig_dev can be the bridge itself. (99b8202b179f) > > 3. (2) broke regular old VLAN configuration, as events generated to > notify the ports that a new VLAN was created in the bridge were now > interpreted as that VLAN being added to the port. > > Which brings us to this series, which tries to put an end to this saga > by reverting (2) and then provides a new fix for that issue which > accepts that orig_dev may be the bridge master, but only for > applicable attributes, and never for switchdev objects. > > I am not really sure about the process here. Is it fine to revert even > if that re-introduces a bug that is then fixed in a followup commit, > or should this be squashed to a single commit? > > Tobias Waldekranz (2): > Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting > ignored" > net: dsa: Always react to global bridge attribute changes > > net/dsa/dsa_priv.h | 10 +--------- > net/dsa/slave.c | 17 +++++++++++++++-- > 2 files changed, 16 insertions(+), 11 deletions(-) > > -- > 2.25.1 > I will take a look at these patches tomorrow, David please don't apply them, I would like to get some sleep first. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-07 10:09 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-06 0:24 [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Tobias Waldekranz 2021-03-06 0:24 ` [PATCH net 1/2] Revert "net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored" Tobias Waldekranz 2021-03-06 0:24 ` [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes Tobias Waldekranz 2021-03-06 14:00 ` Vladimir Oltean 2021-03-06 14:04 ` Vladimir Oltean 2021-03-06 18:17 ` Tobias Waldekranz 2021-03-07 0:58 ` Vladimir Oltean 2021-03-07 9:51 ` Tobias Waldekranz 2021-03-07 10:07 ` Vladimir Oltean 2021-03-06 0:41 ` [PATCH net 0/2] net: dsa: Avoid VLAN config corruption Vladimir Oltean
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.