Date:   Wed,  9 Feb 2022 23:30:41 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <nikolay@...dia.com>,
        Jiri Pirko <jiri@...dia.com>, Ido Schimmel <idosch@...dia.com>,
        Rafael Richter <rafael.richter@....de>,
        Daniel Klauer <daniel.klauer@....de>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: [RFC PATCH net-next 3/5] net: bridge: vlan: notify a switchdev deletion when modifying flags of existing VLAN

There are several reasons why this is desirable.

A user is free to install a bridge VLAN several times in a row, with the
same VID just with the flags changed:

bridge vlan add dev swp0 vid 100 pvid untagged
bridge vlan add dev swp0 vid 100

After the second command runs, a subtle implication is that swp0 no
longer has a pvid, so untagged and priority-tagged traffic received on
that port should be dropped.

A quick survey of the in-tree code base shows that at least some
switchdev drivers do not appear to properly detect this condition, like
for example:

- sparx5_vlan_vid_del() sets port->pvid = 0 if the pvid VLAN was
  deleted, but doesn't set port->pvid = 0 in sparx5_vlan_vid_add() if
  the pvid VLAN was re-added as a non-pvid VLAN

- dpaa2_switch_port_del_vlan() detects the deletion of the pvid VLAN and
  sets the port pvid to 4095. However dpaa2_switch_port_add_vlan() does
  not detect when the pvid VLAN was reinstalled as non-pvid, and doesn't
  have logic to do the same thing as port_del_vlan().

- cpsw_port_vlan_del() calls cpsw_set_pvid(priv, 0, 0, 0), but
  cpsw_port_vlan_add() doesn't.

The list can probably go on. Some drivers handle the condition well, but
the point is that subtle code is easy to get wrong and hence best avoided.

The second argument has to do with driver-level bookkeeping of VLANs.
The goal there, broadly speaking, is to be able to count the number of
VLANs in use as the number of calls to br_switchdev_port_vlan_add()
minus the calls to br_switchdev_port_vlan_del(). This is especially
possible since now we no longer call br_switchdev_port_vlan_add() when
nothing has changed about the VLAN. But when a VLAN is readded with
different flags, the number of additions and deletions is unbalanced.

This is particularly problematic for DSA, who would like to treat VLANs
pointing towards the bridge device as VLANs added on the CPU port.
The CPU port is special because it is a shared resource for all net
devices, in other words VLAN X must be present on the CPU port as long
as:
(a) VLAN X is present in the bridge's VLAN group
(b) at least one switch net device is present as a bridge port

So DSA must reference-count VLANs added on the CPU port (once per switch
net device), and only delete VLAN X once there isn't any switch bridge
port offloading the bridge anymore. Using br_switchdev_port_vlan_{add,del}
is a good indication, except that it is easily thrown off when a VLAN is
reinstalled with different flags. Avoiding this condition at DSA's level
would mean keeping track, for each VLAN, which port it came from, and
what flags it had on that port, to distinguish between a change of flags
(which shouldn't bump the refcount) and a real VLAN addition on a
different net device (which should). Doing this would increase the
memory usage beyond reasonable, when we should be able to make do with a
single refcounting integer, not even the flags are needed.

The presented solution is similar to how FDB entry migration is handled
by commit 90dc8fd36078 ("net: bridge: notify switchdev of disappearance
of old FDB entry upon migration"): when a VLAN is readded with different
flags, first delete it, then readd it with the right ones.

This is a bit against the original spirit of switchdev notifiers which
were supposed to be transactional, but I think part of that was an
excuse to keep the complexity in the bridge driver low, and part was an
overly idealistic view of reality, thinking that complex device drivers
would be any better in terms of bug count. The transactional model for
VLAN offloading was removed in commit ffb68fc58e96 ("net: switchdev:
remove the transaction structure from port object notifiers"), and as
such, the only possible solution in the current API is first to delete,
then add. Treating errors in this model is best-effort, meaning that if
the deletion succeeds, addition fails, we attempt to reinstall old VLAN
with original flags, but that may fail too, but we give up and don't
even attempt to check the error code for the restoration. It isn't
formally perfect, but under the assumption that the original VLAN had
already been accepted once by the driver, and we're holding the
rtnl_mutex, so not much can happen between deletion and addition,
I think there's a good chance that the driver will accept to reinstall
the original VLAN. Plus, there are areas of the bridge driver where
errors from switchdev aren't even checked at all (FDBs), so VLANs
wouldn't be the worst part.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 net/bridge/br_vlan.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 3c149b54124e..5da7e2e23e68 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -747,11 +747,30 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags, bool *changed,
 
 		*changed = brentry_created || flags_changed;
 		if (*changed) {
+			/* First notify to switchdev a deletion of the VLAN
+			 * with the old flags, then an addition with the new
+			 * ones.
+			 */
+			if (flags_changed) {
+				ret = br_switchdev_port_vlan_del(br->dev,
+								 vlan->vid);
+				if (ret && ret != -EOPNOTSUPP) {
+					br_vlan_restore_existing(br, vg, vlan,
+								 old_flags,
+								 brentry_created,
+								 flags_changed);
+					return ret;
+				}
+			}
+
 			ret = br_switchdev_port_vlan_add(br->dev, vlan->vid,
 							 flags, extack);
 			if (ret && ret != -EOPNOTSUPP) {
 				br_vlan_restore_existing(br, vg, vlan, old_flags,
 							 brentry_created, flags_changed);
+				if (flags_changed)
+					br_switchdev_port_vlan_add(br->dev, vlan->vid,
+								   old_flags, NULL);
 				return ret;
 			}
 		}
@@ -1283,10 +1302,18 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 
 		*changed = __vlan_add_flags(vlan, flags);
 		if (*changed) {
-			/* Pass the flags to the hardware bridge */
+			/* Delete the old VLAN and pass the updated flags
+			 * to the hardware bridge.
+			 */
+			ret = br_switchdev_port_vlan_del(port->dev, vid);
+			if (ret && ret != -EOPNOTSUPP)
+				return ret;
+
 			ret = br_switchdev_port_vlan_add(port->dev, vid, flags,
 							 extack);
 			if (ret && ret != -EOPNOTSUPP) {
+				br_switchdev_port_vlan_add(port->dev, vid,
+							   old_flags, NULL);
 				__vlan_add_flags(vlan, old_flags);
 				return ret;
 			}
-- 
2.25.1

Powered by blists - more mailing lists