[PATCH net-next 0/2] net: dsa: fix host fdb refcount when an entry roams

[PATCH net-next 0/2] net: dsa: fix host fdb refcount when an entry roams

[Julius Bairaktaris]

When a bridge FDB entry roams between ports, br_fdb_update() notifies
switchdev of the deletion and the addition without a lock. With two
CPUs learning one address on different ports, DSA counts the host
address on its CPU port more or fewer times than the bridge has it,
which ends in "failed to delete ... from fdb: -2" or an address left on
the CPU port.

The earlier attempt took hash_lock in br_fdb_update(); Nikolay rejected
it because it slows down learning and roaming. This series leaves
br_fdb_update() and the rest of the bridge's learning path unchanged
and fixes DSA instead: each user port records the host addresses it
holds and, on every event, checks where the bridge has the entry now.
Patch 1 is the only bridge change: an RCU lookup that DSA calls from
its workqueue. Patch 2 replaces the counting.

The counting has been exposed to the race since commit 90dc8fd36078
("net: bridge: notify switchdev of disappearance of old FDB entry upon
migration"). The change redesigns how DSA tracks host addresses, so it
goes to net-next without a Fixes: tag.

Tested in QEMU with dsa_loop given FDB operations and assisted
learning, PROVE_LOCKING, PROVE_RCU and DEBUG_ATOMIC_SLEEP enabled, one
source address sent from two veth ports on two CPUs for 5 s, 5 boots
per kernel: net-next logs the -2 error and leaves the address on the
CPU port in 4 of 5 boots, this series in none, and it removes the
address when the user ports leave the bridge. A "bridge fdb replace" of
the address onto a user port leaves it on the CPU port in 5 of 5 boots
on net-next and removes it in 5 of 5 with this series. The kernel log
of every boot has no lockdep, RCU or atomic sleep reports.

Changes since the hash_lock patch:
- Leave br_fdb_update() unchanged (Nikolay).
- Sashiko's finding on that patch, a deletion that races the locked
  roam, has no counterpart here: DSA looks the entry up when it handles
  each event and treats a deleted entry as gone.
- Of the three findings it lists as pre-existing, the MAB and
  BR_FDB_LOCAL ones are in the bridge, which this series does not
  change; the peer-site one, a "bridge fdb replace" onto a DSA user port
  that notifies no deletion, is handled in patch 2.

Link: https://lore.kernel.org/netdev/20261004124254.3525496-1-julius@bairaktaris.de/

I used Claude (Anthropic) to help write these patches.

Julius Bairaktaris (2):
  net: bridge: switchdev: add br_switchdev_fdb_find_port()
  net: dsa: fix host fdb refcount when a bridge fdb entry roams

 include/linux/if_bridge.h |  13 +++
 net/bridge/br_switchdev.c |  63 +++++++++--
 net/dsa/port.c            |  36 +++++--
 net/dsa/port.h            |   6 ++
 net/dsa/user.c            | 216 +++++++++++++++++++++++++++++++++++---
 net/dsa/user.h            |   8 ++
 6 files changed, 309 insertions(+), 33 deletions(-)


base-commit: fa5196392691051963726787944db872420dfa75
-- 
2.53.0

[PATCH net-next 1/2] net: bridge: switchdev: add br_switchdev_fdb_find_port()

[Julius Bairaktaris]

Add an RCU lookup that returns the port of a non-local FDB entry that
switchdev is notified of: not locked, not added by the user as a
dynamic entry, and not learned by a switchdev driver.

A switchdev driver that handles FDB events from a workqueue cannot take
RTNL for br_fdb_find_port(), and has to ignore the entries whose
deletion it will not be notified of. The filter of
br_switchdev_fdb_notify() moves into a helper that both use, with no
functional change; the learning path does not change.

Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 include/linux/if_bridge.h | 13 ++++++++
 net/bridge/br_switchdev.c | 63 +++++++++++++++++++++++++++++++++------
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 75673b8bffcb8..e050b7e698fc9 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -201,6 +201,19 @@ static inline int br_mst_get_state(const struct net_device *dev, u16 msti,
 }
 #endif
 
+#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_NET_SWITCHDEV)
+struct net_device *br_switchdev_fdb_find_port(const struct net_device *br_dev,
+					      const unsigned char *addr,
+					      u16 vid);
+#else
+static inline struct net_device *
+br_switchdev_fdb_find_port(const struct net_device *br_dev,
+			   const unsigned char *addr, u16 vid)
+{
+	return NULL;
+}
+#endif
+
 #if IS_ENABLED(CONFIG_BRIDGE)
 struct net_device *br_fdb_find_port(const struct net_device *br_dev,
 				    const unsigned char *addr,
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 10f3aea6a1f82..02ca1cbcfede0 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -141,14 +141,10 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
 	item->info.ctx = ctx;
 }
 
-void
-br_switchdev_fdb_notify(struct net_bridge *br,
-			const struct net_bridge_fdb_entry *fdb, int type)
+static bool br_switchdev_fdb_notifies(const struct net_bridge_fdb_entry *fdb)
 {
-	struct switchdev_notifier_fdb_info item;
-
 	if (test_bit(BR_FDB_LOCKED, &fdb->flags))
-		return;
+		return false;
 
 	/* Entries with these flags were created using ndm_state == NUD_REACHABLE,
 	 * ndm_flags == NTF_MASTER( | NTF_STICKY), ext_flags == 0 by something
@@ -156,9 +152,58 @@ br_switchdev_fdb_notify(struct net_bridge *br,
 	 * Drivers don't know how to deal with these, so don't notify them to
 	 * avoid confusing them.
 	 */
-	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
-	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
-	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
+	return !test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) ||
+	       test_bit(BR_FDB_STATIC, &fdb->flags) ||
+	       test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
+}
+
+/**
+ * br_switchdev_fdb_find_port - find the port of an entry switchdev knows of
+ * @br_dev: the bridge device
+ * @addr: the MAC address of the entry
+ * @vid: the VLAN of the entry
+ *
+ * Must be called under RCU; the device is valid until the read-side critical
+ * section ends.
+ *
+ * Return: the port of the non-local entry for @addr and @vid, or NULL if
+ * there is none or switchdev is not notified of it.
+ */
+struct net_device *br_switchdev_fdb_find_port(const struct net_device *br_dev,
+					      const unsigned char *addr,
+					      u16 vid)
+{
+	const struct net_bridge_port *p;
+	struct net_bridge_fdb_entry *f;
+
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "suspicious use outside RCU");
+
+	if (!netif_is_bridge_master(br_dev))
+		return NULL;
+
+	f = br_fdb_find_rcu(netdev_priv(br_dev), addr, vid);
+	if (!f || test_bit(BR_FDB_LOCAL, &f->flags) ||
+	    !br_switchdev_fdb_notifies(f))
+		return NULL;
+
+	/* learned by a switchdev driver, which is not notified back */
+	if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
+	    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
+		return NULL;
+
+	p = br_fdb_dst_port(f);
+
+	return p ? p->dev : NULL;
+}
+EXPORT_SYMBOL_GPL(br_switchdev_fdb_find_port);
+
+void
+br_switchdev_fdb_notify(struct net_bridge *br,
+			const struct net_bridge_fdb_entry *fdb, int type)
+{
+	struct switchdev_notifier_fdb_info item;
+
+	if (!br_switchdev_fdb_notifies(fdb))
 		return;
 
 	br_switchdev_fdb_populate(br, &item, fdb, NULL);
-- 
2.53.0

[PATCH net-next 2/2] net: dsa: fix host fdb refcount when a bridge fdb entry roams

[Julius Bairaktaris]

A bridge FDB entry on a foreign port is a host address on every user
port in the bridge. When the entry roams, br_fdb_update() notifies the
deletion from the old port and the addition on the new one without a
lock. With two CPUs receiving the address on different ports, DSA can
get two deletions for one addition, an addition for a port the entry
already left, or the deletion before the addition. Counting these
events either makes a deletion find no reference left on the CPU port,
or leaks a reference, which keeps the address on the CPU port after the
entry moved to a user port or was deleted. The first ends in this
error, here from dsa_loop:

  port 1 failed to delete 02:5a:0b:a2:1a:46 vid 0 from fdb: -2

Instead of counting, each user port records the host addresses it
holds, and on every event holds the address while the bridge has the
entry on a foreign port and notifies switchdev of it. A deletion that
still finds the entry on its own port was sent before the bridge
changed the port, so the address is released. An entry on a user port,
also one of another tree, is notified only to that port and is not
held; an addition there releases the address on the other ports of the
bridge, which also covers a "bridge fdb replace" onto a user port that
notifies no deletion. Replayed deletions release the address without
the lookup. When the port leaves the bridge, or fails to join it, the
workqueue is flushed before the bridge is freed and the records left
are released. Addresses local to the bridge do not roam and are still
counted. The software bridge is not changed.

In QEMU, with dsa_loop given FDB operations and assisted learning, two
veth ports in its bridge and one source address sent from both on two
CPUs for 5 s, the deletion fails and the address stays on the CPU port
after both veth ports go down in 4 of 5 boots. With this change neither
happens in 5 of 5 boots, and the address is removed when the user ports
leave the bridge. A "bridge fdb replace" of the address onto a user port
leaves it on the CPU port in 5 of 5 boots without this change and
removes it in 5 of 5 with it.

Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 net/dsa/port.c |  36 +++++++--
 net/dsa/port.h |   6 ++
 net/dsa/user.c | 216 +++++++++++++++++++++++++++++++++++++++++++++----
 net/dsa/user.h |   8 ++
 4 files changed, 242 insertions(+), 24 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 1f5536c0dffc2..e270a0c5e6641 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -457,7 +457,13 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp,
 {
 	struct dsa_bridge *bridge = dp->bridge;
 
-	dp->bridge = NULL;
+	WRITE_ONCE(dp->bridge, NULL);
+
+	/* Wait for the FDB events that still see the bridge, then release the
+	 * host addresses the port holds, before the drivers are told it left.
+	 */
+	dsa_flush_workqueue();
+	dsa_user_bridge_host_fdb_purge(dp, bridge);
 
 	if (!refcount_dec_and_test(&bridge->refcount))
 		return;
@@ -528,7 +534,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
 					&dsa_user_switchdev_blocking_notifier);
 	dsa_flush_workqueue();
 out_rollback_unbridge:
+	dsa_port_bridge_destroy(dp, br);
 	dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
+	return err;
 out_rollback:
 	dsa_port_bridge_destroy(dp, br);
 	return err;
@@ -1040,13 +1048,14 @@ int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 	return dsa_port_host_fdb_add(dp, addr, vid, db);
 }
 
-int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
-				 const unsigned char *addr, u16 vid)
+int __dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
+				   const struct dsa_bridge *bridge,
+				   const unsigned char *addr, u16 vid)
 {
 	struct net_device *conduit = dsa_port_to_conduit(dp);
 	struct dsa_db db = {
 		.type = DSA_DB_BRIDGE,
-		.bridge = *dp->bridge,
+		.bridge = *bridge,
 	};
 	int err;
 
@@ -1066,6 +1075,12 @@ int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
 	return dsa_port_host_fdb_add(dp, addr, vid, db);
 }
 
+int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
+				 const unsigned char *addr, u16 vid)
+{
+	return __dsa_port_bridge_host_fdb_add(dp, dp->bridge, addr, vid);
+}
+
 static int dsa_port_host_fdb_del(struct dsa_port *dp,
 				 const unsigned char *addr, u16 vid,
 				 struct dsa_db db)
@@ -1091,13 +1106,14 @@ int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
 	return dsa_port_host_fdb_del(dp, addr, vid, db);
 }
 
-int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
-				 const unsigned char *addr, u16 vid)
+int __dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
+				   const struct dsa_bridge *bridge,
+				   const unsigned char *addr, u16 vid)
 {
 	struct net_device *conduit = dsa_port_to_conduit(dp);
 	struct dsa_db db = {
 		.type = DSA_DB_BRIDGE,
-		.bridge = *dp->bridge,
+		.bridge = *bridge,
 	};
 	int err;
 
@@ -1113,6 +1129,12 @@ int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
 	return dsa_port_host_fdb_del(dp, addr, vid, db);
 }
 
+int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
+				 const unsigned char *addr, u16 vid)
+{
+	return __dsa_port_bridge_host_fdb_del(dp, dp->bridge, addr, vid);
+}
+
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 			 u16 vid)
 {
diff --git a/net/dsa/port.h b/net/dsa/port.h
index 6bc3291573c02..6524f67a37a20 100644
--- a/net/dsa/port.h
+++ b/net/dsa/port.h
@@ -54,8 +54,14 @@ int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
 int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
+int __dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
+				   const struct dsa_bridge *bridge,
+				   const unsigned char *addr, u16 vid);
 int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 				 u16 vid);
+int __dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
+				   const struct dsa_bridge *bridge,
+				   const unsigned char *addr, u16 vid);
 int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 				 u16 vid);
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 041f9060c8ef0..7f18c7885b9c5 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -43,6 +43,8 @@ struct dsa_switchdev_event_work {
 	unsigned char addr[ETH_ALEN];
 	u16 vid;
 	bool host_addr;
+	bool sync;
+	bool replay;
 };
 
 enum dsa_standalone_event {
@@ -2813,6 +2815,7 @@ int dsa_user_create(struct dsa_port *port)
 
 	p->dp = port;
 	INIT_LIST_HEAD(&p->mall_tc_list);
+	INIT_LIST_HEAD(&p->bridge_host_fdbs);
 	port->user = user_dev;
 	dsa_user_setup_tagger(user_dev);
 
@@ -3627,6 +3630,22 @@ static int dsa_user_netdevice_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static bool dsa_foreign_dev_check(const struct net_device *dev,
+				  const struct net_device *foreign_dev)
+{
+	const struct dsa_port *dp = dsa_user_to_port(dev);
+	struct dsa_switch_tree *dst = dp->ds->dst;
+
+	if (netif_is_bridge_master(foreign_dev))
+		return !dsa_tree_offloads_bridge_dev(dst, foreign_dev);
+
+	if (netif_is_bridge_port(foreign_dev))
+		return !dsa_tree_offloads_bridge_port(dst, foreign_dev);
+
+	/* Everything else is foreign */
+	return true;
+}
+
 static void
 dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
 {
@@ -3639,6 +3658,171 @@ dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
 				 switchdev_work->orig_dev, &info.info, NULL);
 }
 
+/* The bridge notifies a roaming entry without a lock, so with two CPUs the
+ * deletion from the old port and the addition on the new one can arrive
+ * twice or out of order. Hold the host address while the bridge has the
+ * entry on a foreign port. A deletion that still finds the entry on its own
+ * port was sent before the bridge changed the port. An entry on a user port,
+ * also one of another tree, is notified only to that port.
+ */
+static bool dsa_user_bridge_host_fdb_wanted(struct net_device *dev,
+					    const struct dsa_bridge *bridge,
+					    const unsigned char *addr, u16 vid,
+					    const struct net_device *leaving)
+{
+	struct net_device *brport;
+	bool want;
+
+	rcu_read_lock();
+	brport = br_switchdev_fdb_find_port(bridge->dev, addr, vid);
+	want = brport && brport != leaving && !dsa_user_dev_check(brport) &&
+	       dsa_foreign_dev_check(dev, brport);
+	rcu_read_unlock();
+
+	return want;
+}
+
+static void dsa_user_bridge_host_fdb_set(struct net_device *dev,
+					 const struct dsa_bridge *bridge,
+					 const unsigned char *addr, u16 vid,
+					 bool want)
+{
+	struct dsa_user_priv *p = netdev_priv(dev);
+	struct dsa_port *dp = p->dp;
+	struct dsa_mac_addr *a;
+	bool held = false;
+	int err;
+
+	list_for_each_entry(a, &p->bridge_host_fdbs, list) {
+		if (ether_addr_equal(a->addr, addr) && a->vid == vid) {
+			held = true;
+			break;
+		}
+	}
+
+	if (want == held)
+		return;
+
+	if (want) {
+		a = kzalloc_obj(*a);
+		if (!a)
+			return;
+
+		err = __dsa_port_bridge_host_fdb_add(dp, bridge, addr, vid);
+		if (err) {
+			dev_err(dp->ds->dev,
+				"port %d failed to add %pM vid %d to fdb: %d\n",
+				dp->index, addr, vid, err);
+			kfree(a);
+			return;
+		}
+
+		ether_addr_copy(a->addr, addr);
+		a->vid = vid;
+		list_add(&a->list, &p->bridge_host_fdbs);
+	} else {
+		err = __dsa_port_bridge_host_fdb_del(dp, bridge, addr, vid);
+		if (err) {
+			dev_err(dp->ds->dev,
+				"port %d failed to delete %pM vid %d from fdb: %d\n",
+				dp->index, addr, vid, err);
+			return;
+		}
+
+		list_del(&a->list);
+		kfree(a);
+	}
+}
+
+static void
+dsa_user_bridge_host_fdb_sync(struct dsa_switchdev_event_work *switchdev_work)
+{
+	bool del = switchdev_work->event == SWITCHDEV_FDB_DEL_TO_DEVICE;
+	struct net_device *dev = switchdev_work->dev;
+	const struct dsa_bridge *bridge;
+	bool want = false;
+
+	/* Cleared before the workqueue is flushed and the bridge is freed */
+	bridge = READ_ONCE(dsa_user_to_port(dev)->bridge);
+	if (!bridge)
+		return;
+
+	if (!(del && switchdev_work->replay))
+		want = dsa_user_bridge_host_fdb_wanted(dev, bridge,
+						       switchdev_work->addr,
+						       switchdev_work->vid,
+						       del ? switchdev_work->orig_dev : NULL);
+
+	dsa_user_bridge_host_fdb_set(dev, bridge, switchdev_work->addr,
+				     switchdev_work->vid, want);
+	if (want && !del)
+		dsa_fdb_offload_notify(switchdev_work);
+}
+
+/* Only the user port an entry is on gets its events, so when the entry roams
+ * there, release it on the ports of the bridge that still hold it.
+ */
+static void dsa_user_bridge_host_fdb_release_work(struct work_struct *work)
+{
+	struct dsa_switchdev_event_work *switchdev_work =
+		container_of(work, struct dsa_switchdev_event_work, work);
+	struct dsa_port *dp = dsa_user_to_port(switchdev_work->dev);
+	const unsigned char *addr = switchdev_work->addr;
+	const struct dsa_bridge *bridge = READ_ONCE(dp->bridge);
+	u16 vid = switchdev_work->vid;
+	struct dsa_port *other;
+
+	if (bridge) {
+		dsa_tree_for_each_user_port(other, dp->ds->dst) {
+			if (dsa_port_offloads_bridge_dev(other, bridge->dev) &&
+			    !dsa_user_bridge_host_fdb_wanted(other->user, bridge,
+							     addr, vid, NULL))
+				dsa_user_bridge_host_fdb_set(other->user, bridge,
+							     addr, vid, false);
+		}
+	}
+
+	kfree(switchdev_work);
+}
+
+static int dsa_user_bridge_host_fdb_release(struct net_device *dev,
+					    const unsigned char *addr, u16 vid)
+{
+	struct dsa_switchdev_event_work *switchdev_work;
+
+	switchdev_work = kzalloc_obj(*switchdev_work, GFP_ATOMIC);
+	if (!switchdev_work)
+		return -ENOMEM;
+
+	INIT_WORK(&switchdev_work->work, dsa_user_bridge_host_fdb_release_work);
+	switchdev_work->dev = dev;
+	ether_addr_copy(switchdev_work->addr, addr);
+	switchdev_work->vid = vid;
+
+	dsa_schedule_work(&switchdev_work->work);
+
+	return 0;
+}
+
+void dsa_user_bridge_host_fdb_purge(struct dsa_port *dp,
+				    const struct dsa_bridge *bridge)
+{
+	struct dsa_user_priv *p = netdev_priv(dp->user);
+	struct dsa_mac_addr *a, *tmp;
+	int err;
+
+	list_for_each_entry_safe(a, tmp, &p->bridge_host_fdbs, list) {
+		err = __dsa_port_bridge_host_fdb_del(dp, bridge, a->addr,
+						     a->vid);
+		if (err)
+			dev_err(dp->ds->dev,
+				"port %d failed to delete %pM vid %d from fdb: %d\n",
+				dp->index, a->addr, a->vid, err);
+		list_del(&a->list);
+		kfree(a);
+	}
+}
+
 static void dsa_user_switchdev_event_work(struct work_struct *work)
 {
 	struct dsa_switchdev_event_work *switchdev_work =
@@ -3653,6 +3837,12 @@ static void dsa_user_switchdev_event_work(struct work_struct *work)
 	dp = dsa_user_to_port(dev);
 	ds = dp->ds;
 
+	if (switchdev_work->sync) {
+		dsa_user_bridge_host_fdb_sync(switchdev_work);
+		kfree(switchdev_work);
+		return;
+	}
+
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 		if (switchdev_work->host_addr)
@@ -3689,22 +3879,6 @@ static void dsa_user_switchdev_event_work(struct work_struct *work)
 	kfree(switchdev_work);
 }
 
-static bool dsa_foreign_dev_check(const struct net_device *dev,
-				  const struct net_device *foreign_dev)
-{
-	const struct dsa_port *dp = dsa_user_to_port(dev);
-	struct dsa_switch_tree *dst = dp->ds->dst;
-
-	if (netif_is_bridge_master(foreign_dev))
-		return !dsa_tree_offloads_bridge_dev(dst, foreign_dev);
-
-	if (netif_is_bridge_port(foreign_dev))
-		return !dsa_tree_offloads_bridge_port(dst, foreign_dev);
-
-	/* Everything else is foreign */
-	return true;
-}
-
 static int dsa_user_fdb_event(struct net_device *dev,
 			      struct net_device *orig_dev,
 			      unsigned long event, const void *ctx,
@@ -3723,7 +3897,9 @@ static int dsa_user_fdb_event(struct net_device *dev,
 
 	if (switchdev_fdb_is_dynamically_learned(fdb_info)) {
 		if (dsa_port_offloads_bridge_port(dp, orig_dev))
-			return 0;
+			return event == SWITCHDEV_FDB_ADD_TO_DEVICE ?
+			       dsa_user_bridge_host_fdb_release(dev, fdb_info->addr,
+								fdb_info->vid) : 0;
 
 		/* FDB entries learned by the software bridge or by foreign
 		 * bridge ports should be installed as host addresses only if
@@ -3768,9 +3944,15 @@ static int dsa_user_fdb_event(struct net_device *dev,
 	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
 	switchdev_work->vid = fdb_info->vid;
 	switchdev_work->host_addr = host_addr;
+	switchdev_work->sync = host_addr && !fdb_info->is_local;
+	switchdev_work->replay = !!ctx;
 
 	dsa_schedule_work(&switchdev_work->work);
 
+	if (!host_addr && event == SWITCHDEV_FDB_ADD_TO_DEVICE)
+		return dsa_user_bridge_host_fdb_release(dev, fdb_info->addr,
+							fdb_info->vid);
+
 	return 0;
 }
 
diff --git a/net/dsa/user.h b/net/dsa/user.h
index 016884bead3cd..dd38104162da4 100644
--- a/net/dsa/user.h
+++ b/net/dsa/user.h
@@ -33,6 +33,12 @@ struct dsa_user_priv {
 
 	/* TC context */
 	struct list_head	mall_tc_list;
+
+	/* Host addresses of bridge FDB entries held through this port,
+	 * changed from the DSA workqueue, or with the port out of the bridge
+	 * and the workqueue flushed
+	 */
+	struct list_head	bridge_host_fdbs;
 };
 
 void dsa_user_mii_bus_init(struct dsa_switch *ds);
@@ -47,6 +53,8 @@ void dsa_user_host_uc_uninstall(struct net_device *dev);
 void dsa_user_sync_ha(struct net_device *dev);
 void dsa_user_unsync_ha(struct net_device *dev);
 void dsa_user_setup_tagger(struct net_device *user);
+void dsa_user_bridge_host_fdb_purge(struct dsa_port *dp,
+				    const struct dsa_bridge *bridge);
 int dsa_user_change_mtu(struct net_device *dev, int new_mtu);
 int dsa_user_change_conduit(struct net_device *dev, struct net_device *conduit,
 			    struct netlink_ext_ack *extack);
-- 
2.53.0

Re: [PATCH net-next 0/2] net: dsa: fix host fdb refcount when an entry roams

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next 0/2] net: dsa: fix host fdb refcount when an entry roams

[Julius Bairaktaris]

> - How the issue was discovered, e.g. hit in production, hit during
>   development, syzbot report, manual code inspection, LLM or static
>   analysis tool scan.

Hit during development on a Xiaomi AX3600 (IPQ8074) running OpenWrt's
qca-ppe DSA switch driver. A station roaming between a DSA user port
and a Wi-Fi AP port of the same bridge logged

  qca-ppe 3a000000.ppe: port 5 failed to delete 02:5a:0b:a2:1a:46 vid 0 from fdb: -2

3-6 times per address when the two ports received on different CPUs,
and not at all when they shared one CPU (4 runs each). The dsa_loop
harness described in the cover letter reproduces it on net-next.

Julius

Re: [PATCH net-next 1/2] net: bridge: switchdev: add br_switchdev_fdb_find_port()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] br_switchdev_fdb_find_port() has a kernel-doc that says it
  returns NULL when "switchdev is not notified of" the entry.
- [Low] Concern: br_switchdev_fdb_find_port() is exported, declared in
  include/linux/if_bridge.h and given a stub there, but nothing in the…

--

net: bridge: switchdev: add br_switchdev_fdb_find_port()

This commit adds br_switchdev_fdb_find_port(), an exported RCU lookup
that returns the port of a non-local bridge FDB entry that switchdev is
notified of. The filter in br_switchdev_fdb_notify() moves into a new
helper, br_switchdev_fdb_notifies(), and both functions use it.

> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 75673b8bffcb8..e050b7e698fc9 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -201,6 +201,19 @@ static inline int br_mst_get_state(const struct net_device *dev, u16 msti,
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_NET_SWITCHDEV)
> +struct net_device *br_switchdev_fdb_find_port(const struct net_device *br_dev,
> +					      const unsigned char *addr,
> +					      u16 vid);

[Severity: Low]
This isn't a bug, but nothing in this patch calls the newly exported
br_switchdev_fdb_find_port().

The only user seems to come in the next patch of the series, "net: dsa:
fix host fdb refcount when a bridge fdb entry roams". That patch calls it
from dsa_user_bridge_host_fdb_wanted() in net/dsa/user.c:

    brport = br_switchdev_fdb_find_port(bridge->dev, addr, vid);

Could the commit message name that consumer?

> +#else

[ ... ]

> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 10f3aea6a1f82..02ca1cbcfede0 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c

[ ... ]

> @@ -156,9 +152,58 @@ br_switchdev_fdb_notify(struct net_bridge *br,
>  	 * Drivers don't know how to deal with these, so don't notify them to
>  	 * avoid confusing them.
>  	 */
> -	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
> -	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
> -	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
> +	return !test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) ||
> +	       test_bit(BR_FDB_STATIC, &fdb->flags) ||
> +	       test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
> +}
> +
> +/**
> + * br_switchdev_fdb_find_port - find the port of an entry switchdev knows of
> + * @br_dev: the bridge device
> + * @addr: the MAC address of the entry
> + * @vid: the VLAN of the entry
> + *
> + * Must be called under RCU; the device is valid until the read-side critical
> + * section ends.
> + *
> + * Return: the port of the non-local entry for @addr and @vid, or NULL if
> + * there is none or switchdev is not notified of it.
> + */
> +struct net_device *br_switchdev_fdb_find_port(const struct net_device *br_dev,
> +					      const unsigned char *addr,
> +					      u16 vid)
> +{
> +	const struct net_bridge_port *p;
> +	struct net_bridge_fdb_entry *f;
> +
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "suspicious use outside RCU");
> +
> +	if (!netif_is_bridge_master(br_dev))
> +		return NULL;
> +
> +	f = br_fdb_find_rcu(netdev_priv(br_dev), addr, vid);
> +	if (!f || test_bit(BR_FDB_LOCAL, &f->flags) ||
> +	    !br_switchdev_fdb_notifies(f))
> +		return NULL;
> +
> +	/* learned by a switchdev driver, which is not notified back */
> +	if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> +	    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
> +		return NULL;

[Severity: Medium]
Does this filter match the kernel-doc above? The kernel-doc says NULL is
returned when "switchdev is not notified of" the entry.

The commit message also says the caller uses it to "ignore the entries
whose deletion it will not be notified of". Several cases seem to
contradict both statements.

The comment "learned by a switchdev driver, which is not notified back"
only holds for the addition. br_switchdev_fdb_notifies() returns true for
BR_FDB_ADDED_BY_EXT_LEARN entries. So a flush, a delete-by-port or
__br_fdb_delete() still sends SWITCHDEV_FDB_DEL_TO_DEVICE for them,
because those paths go through fdb_delete(br, f, true).

A software-learned entry has already had SWITCHDEV_FDB_ADD_TO_DEVICE
sent. Suppose a driver then reports the same addr/vid with
SWITCHDEV_FDB_ADD_TO_BRIDGE. The existing-entry branch of
br_fdb_external_learn_add() does:

    if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {

It leaves BR_FDB_ADDED_BY_USER clear. This helper then returns NULL for
an entry that switchdev already knows about.

Replay also skips the filter. br_switchdev_fdb_replay_one() notifies
every fdb_list entry without calling br_switchdev_fdb_notifies():

    br_switchdev_fdb_populate(br, &item, fdb, ctx);

    err = nb->notifier_call(nb, action, &item);

Locked entries and user "master dynamic" entries are therefore notified
on replay, but this helper reports them as not notified.

The opposite case looks more concerning. A user-added extern_learn entry
(bridge fdb add ... master extern_learn) goes through
__br_fdb_add()->br_fdb_external_learn_add(..., false, true). It gets
BR_FDB_ADDED_BY_USER | BR_FDB_ADDED_BY_EXT_LEARN and passes this filter.

Any switchdev driver can then delete that entry without a DEL_TO_DEVICE
notification:

br_switchdev_event(SWITCHDEV_FDB_DEL_TO_BRIDGE)
  br_fdb_external_learn_del(br, p, addr, vid, false)
    fdb_delete(br, fdb, false)

br_fdb_external_learn_del() only checks BR_FDB_ADDED_BY_EXT_LEARN, not
the port:

    if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
        fdb_delete(br, fdb, swdev_notify);

With the DSA consumer from the next patch, consider a user port that
holds a host address for such an entry on a foreign port. Would it ever
get an event telling it to drop that address? The address seems to stay
on the CPU port until a later event for that addr/vid, or until the port
leaves the bridge.

Should the kernel-doc and commit message describe what the filter really
guarantees, or should the filter change to match the stated contract?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794134495.git.julius%40bairaktaris.de

Re: [PATCH net-next 2/2] net: dsa: fix host fdb refcount when a bridge fdb entry roams

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 11 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 7 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix does not cover cross-tree bridges, although the commit
  message says it does.
- [High] The join rollback path out_rollback_unbridge now dereferences
  NULL in queued work.
- [Medium] The commit message says "Addresses local to the bridge do not
  roam and are still counted." That is wrong for `bridge fdb replace`.
- [Medium] dsa_user_bridge_host_fdb_sync() sends SWITCHDEV_FDB_OFFLOADED
  for every ADD where want is true, even when the host address was not…
- [Medium] Retrying a failed host delete drops the conduit's unicast-
  filter reference twice.
- [Medium] Every dynamically learned ADD on a DSA user port now costs a
  GFP_ATOMIC allocation and a work item, on every DSA switch.
- [Medium] On a "bridge fdb replace" of an address from a foreign port
  onto a DSA user port, the user's new static entry is erased from…
- [Medium] net: dsa: dsa_user_bridge_host_fdb_wanted() ignores
  assisted_learning_on_cpu_port
- [Medium] net: dsa: dsa_user_fdb_event() skips host FDB release on LAG
  without lag_fdb_add
- [Low] The patch fixes a user-visible bug (`port N failed to delete ...
  from fdb: -2` and host addresses left on the CPU port) but has no…
- [Low] The patch turns dp->bridge into a lockless clear-then-flush
  protocol: WRITE_ONCE(dp->bridge, NULL), then dsa_flush_workqueue(),…

--

net: dsa: fix host fdb refcount when a bridge fdb entry roams

This replaces the counted host address handling for bridge FDB entries
on foreign ports with a per-user-port list of held addresses. The list
is re-evaluated against the bridge FDB on every event. Additions on a
user port schedule a release of the address on the other ports of the
bridge, and dsa_port_bridge_destroy() now flushes the workqueue and
purges the held records.

> are released. Addresses local to the bridge do not roam and are still
> counted. The software bridge is not changed.

[ ... ]

> Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>

[Severity: Low]
This fixes a user-visible error ("port 1 failed to delete ... from fdb:
-2") and a host address left on the CPU port, but there is no Fixes:
tag. Could one be added so stable and backporters can tell where this
applies?

The fix also depends on br_switchdev_fdb_find_port(), which
dsa_user_bridge_host_fdb_wanted() calls. That helper is added by the
preceding bridge commit ("net: bridge: switchdev: add
br_switchdev_fdb_find_port()").

Given "The software bridge is not changed" in the message, could that
dependency be stated explicitly?

> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 1f5536c0dffc2..e270a0c5e6641 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -457,7 +457,13 @@ static void dsa_port_bridge_destroy(struct dsa_port *dp,
>  {
>  	struct dsa_bridge *bridge = dp->bridge;
>  
> -	dp->bridge = NULL;
> +	WRITE_ONCE(dp->bridge, NULL);
> +
> +	/* Wait for the FDB events that still see the bridge, then release the
> +	 * host addresses the port holds, before the drivers are told it left.
> +	 */
> +	dsa_flush_workqueue();
> +	dsa_user_bridge_host_fdb_purge(dp, bridge);
>  
>  	if (!refcount_dec_and_test(&bridge->refcount))
>  		return;
> @@ -528,7 +534,9 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>  					&dsa_user_switchdev_blocking_notifier);
>  	dsa_flush_workqueue();
>  out_rollback_unbridge:
> +	dsa_port_bridge_destroy(dp, br);
>  	dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
> +	return err;
>  out_rollback:
>  	dsa_port_bridge_destroy(dp, br);
>  	return err;

[Severity: High]
Can the pending FDB works dereference a NULL bridge on this rollback
path?

out_rollback_unbridge is reached when switchdev_bridge_port_offload()
fails, and nothing unoffloads or flushes first. Its FDB replay
(br_switchdev_fdb_replay()) delivers every bridge FDB entry with
ctx = dp. Local entries such as the bridge and port MACs reach
dsa_user_fdb_event() with is_local set, so they get host_addr = true
and sync = false. A plain dsa_user_switchdev_event_work() item is
queued for each one.

If a later replayed entry fails, the replay stops and the error comes
back here. One way this happens is the GFP_ATOMIC allocation in
dsa_user_fdb_event(). Another is the new
dsa_user_bridge_host_fdb_release(), which returns -ENOMEM after a work
item has already been queued.

dsa_port_bridge_destroy() now does WRITE_ONCE(dp->bridge, NULL) before
dsa_flush_workqueue(). So every work still pending runs with
dp->bridge == NULL, and only the new sync branch checks for that. The
non-sync ADD branch does:

dsa_user_switchdev_event_work()
  dsa_port_bridge_host_fdb_add(dp, addr, vid)
    __dsa_port_bridge_host_fdb_add(dp, dp->bridge, addr, vid)
      .bridge = *bridge,

dsa_port_fdb_add() also evaluates *dp->bridge.

The commit message says the workqueue is flushed when the port "fails
to join" the bridge. Does the non-sync path need a NULL bridge check
too, or should the flush happen before dp->bridge is cleared?

[ ... ]

> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 041f9060c8ef0..7f18c7885b9c5 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c

[ ... ]

> @@ -3639,6 +3658,171 @@ dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
>  				 switchdev_work->orig_dev, &info.info, NULL);
>  }
>  

[ ... ]

> +static void dsa_user_bridge_host_fdb_set(struct net_device *dev,
> +					 const struct dsa_bridge *bridge,
> +					 const unsigned char *addr, u16 vid,
> +					 bool want)
> +{

[ ... ]

> +	} else {
> +		err = __dsa_port_bridge_host_fdb_del(dp, bridge, addr, vid);
> +		if (err) {
> +			dev_err(dp->ds->dev,
> +				"port %d failed to delete %pM vid %d from fdb: %d\n",
> +				dp->index, addr, vid, err);
> +			return;
> +		}

[Severity: Medium]
After a failed delete the record is kept. Can that call dev_uc_del()
on the conduit more than once for a single dev_uc_add()?

__dsa_port_bridge_host_fdb_del() does:

	if (conduit->priv_flags & IFF_UNICAST_FLT) {
		err = dev_uc_del(conduit, addr);
		if (err)
			return err;
	}

	return dsa_port_host_fdb_del(dp, addr, vid, db);

Suppose dev_uc_del() succeeds and dsa_port_host_fdb_del() fails, for
example with a driver port_fdb_del() error. The dev_uc_del() is not
undone, but the record stays on bridge_host_fdbs. A later want == false
event, or dsa_user_bridge_host_fdb_purge(), then repeats the whole
delete, including another dev_uc_del().

The conduit uc list is refcounted per MAC and shared across VIDs,
bridges and other host address users. The extra delete can drop
another holder's reference.

When that holder later releases, dev_uc_del() returns -ENOENT, and
__dsa_port_bridge_host_fdb_del() returns before the switch delete.
dsa_user_bridge_host_fdb_purge() then logs the error and frees the
record anyway:

	err = __dsa_port_bridge_host_fdb_del(dp, bridge, a->addr,
					     a->vid);
	if (err)
		dev_err(...);
	list_del(&a->list);
	kfree(a);

Would the CPU port FDB entry then stay programmed, with nothing left
to remove it?

> +
> +		list_del(&a->list);
> +		kfree(a);
> +	}
> +}
> +
> +static void
> +dsa_user_bridge_host_fdb_sync(struct dsa_switchdev_event_work *switchdev_work)
> +{

[ ... ]

> +	dsa_user_bridge_host_fdb_set(dev, bridge, switchdev_work->addr,
> +				     switchdev_work->vid, want);
> +	if (want && !del)
> +		dsa_fdb_offload_notify(switchdev_work);
> +}

[Severity: Medium]
Is SWITCHDEV_FDB_OFFLOADED sent here even when the host address was not
installed?

dsa_user_bridge_host_fdb_set() returns void. It can return early
without installing anything:

	if (want) {
		a = kzalloc_obj(*a);
		if (!a)
			return;

It does the same when __dsa_port_bridge_host_fdb_add() fails.

The caller still calls dsa_fdb_offload_notify() whenever want is true,
so the bridge marks the entry offloaded. Before this patch, the error
path in dsa_user_switchdev_event_work() did a break and skipped the
notification.

Could dsa_user_bridge_host_fdb_set() report whether the address ended
up held, so the notification is only sent then?

> +
> +/* Only the user port an entry is on gets its events, so when the entry roams
> + * there, release it on the ports of the bridge that still hold it.
> + */
> +static void dsa_user_bridge_host_fdb_release_work(struct work_struct *work)
> +{
> +	struct dsa_switchdev_event_work *switchdev_work =
> +		container_of(work, struct dsa_switchdev_event_work, work);
> +	struct dsa_port *dp = dsa_user_to_port(switchdev_work->dev);
> +	const unsigned char *addr = switchdev_work->addr;
> +	const struct dsa_bridge *bridge = READ_ONCE(dp->bridge);
> +	u16 vid = switchdev_work->vid;
> +	struct dsa_port *other;
> +
> +	if (bridge) {
> +		dsa_tree_for_each_user_port(other, dp->ds->dst) {

[Severity: High]
Does this release reach the user ports of other trees in the same
bridge?

The commit message says an entry on a user port, "also one of another
tree", is not held. It also says "an addition there releases the
address on the other ports of the bridge". This loop only walks
dp->ds->dst, which is the tree of the port that got the ADD.

For an entry on a DSA user port,
__switchdev_handle_fdb_event_to_device() only delivers the event to
that port:

	if (check_cb(dev))
		return mod_cb(dev, orig_dev, event, info->ctx, fdb_info);

Take trees A and B in one bridge, with ADDR on a foreign port such as
a veth. Every user port of both trees holds ADDR. Then run this, where
swpB is a tree B user port:

  bridge fdb replace ADDR dev swpB master static

fdb_add_entry() only notifies RTM_NEWNEIGH. Only swpB gets the event,
and the release work only visits tree B. Tree A's ports never call
dsa_user_bridge_host_fdb_set(..., false).

Would ADDR stay on tree A's CPU port until those ports leave the
bridge, even after the entry is deleted? Should this walk every tree
in dsa_tree_list instead?

> +			if (dsa_port_offloads_bridge_dev(other, bridge->dev) &&
> +			    !dsa_user_bridge_host_fdb_wanted(other->user, bridge,
> +							     addr, vid, NULL))
> +				dsa_user_bridge_host_fdb_set(other->user, bridge,
> +							     addr, vid, false);

[Severity: Low]
Do these reads of the other ports' dp->bridge race with
dsa_port_bridge_destroy() on those ports?

dsa_port_bridge_destroy() clears dp->bridge with WRITE_ONCE() before it
flushes the workqueue. So this work can run while port "other" is
leaving the bridge.

dsa_port_offloads_bridge_dev() goes through dsa_port_bridge_dev_get():

	return dp->bridge ? dp->bridge->dev : NULL;

That is two plain loads. dsa_user_bridge_host_fdb_wanted() reaches
similar plain reads through dsa_foreign_dev_check() ->
dsa_tree_offloads_bridge_dev() / dsa_tree_offloads_bridge_port(). On
the writer side, dsa_port_bridge_create() still publishes with a plain
dp->bridge = bridge.

KCSAN would report these races. If the compiler reloads dp->bridge
between the check and the dereference, the dereference can see NULL.

Should the remaining dp->bridge accesses in this scheme be annotated
too?

> +		}
> +	}
> +
> +	kfree(switchdev_work);
> +}

[ ... ]

> @@ -3723,7 +3897,9 @@ static int dsa_user_fdb_event(struct net_device *dev,
>  
>  	if (switchdev_fdb_is_dynamically_learned(fdb_info)) {
>  		if (dsa_port_offloads_bridge_port(dp, orig_dev))
> -			return 0;
> +			return event == SWITCHDEV_FDB_ADD_TO_DEVICE ?
> +			       dsa_user_bridge_host_fdb_release(dev, fdb_info->addr,
> +								fdb_info->vid) : 0;

[Severity: Medium]
When ds->assisted_learning_on_cpu_port is not set, can this keep a
host address held that nothing will ever release?

dsa_user_fdb_event() drops dynamically learned entries on foreign
ports, both ADD and DEL, when assisted_learning_on_cpu_port is false.
User-added entries on foreign ports still go through
dsa_user_bridge_host_fdb_sync().

The lookup here does not make that distinction.
br_switchdev_fdb_find_port() returns the port of any non-local entry,
including one the bridge learned dynamically.

Consider a static entry for ADDR on veth0, where veth0 and veth1 are
both foreign ports of the bridge. Then run:

  bridge fdb del ADDR dev veth0 master

Suppose a frame from ADDR arrives on veth1 before the DEL work runs on
dsa_owq, and the bridge learns ADDR there. That ADD is dynamically
learned, so dsa_user_fdb_event() returns 0 for it.

When the DEL work then calls dsa_user_bridge_host_fdb_wanted(), brport
is veth1. veth1 is not leaving, is not a DSA user port, and is
foreign. So want is true and the record on bridge_host_fdbs is kept.

When the dynamic entry on veth1 ages out, its DEL is dropped by the
same assisted_learning_on_cpu_port check, so no sync work is queued.
The address stays on the CPU port until the user ports leave the
bridge.

Should dsa_user_bridge_host_fdb_wanted() apply the same filter as
dsa_user_fdb_event()? For example, the lookup could also report
whether the entry was dynamically learned. Or should DEL events for
addresses a port holds bypass that filter?

[Severity: Medium]
Does every dynamically learned ADD on a user port now cost a GFP_ATOMIC
allocation and a work item, on every DSA switch?

Before this patch, this branch returned 0 right away. It now runs
before the ds->assisted_learning_on_cpu_port check. So it applies even
when no port holds any bridge host address and the work has nothing to
do.

Each release work walks every user port of the tree. For each bridged
port it does an RCU FDB lookup and a linear scan of bridge_host_fdbs.
All of this runs on the single ordered dsa_owq. Non-host static ADDs
also queue two work items, the normal one and the release.

An L2 peer on a user port can send frames with changing source MACs.
The bridge learns each one, since the default fdb_max_learned of 0
means no limit. Each learn then queues work that competes with real
FDB programming on the ordered workqueue.

Is this cost intended for switches that never hold bridge host
addresses?

>  
>  		/* FDB entries learned by the software bridge or by foreign
>  		 * bridge ports should be installed as host addresses only if
> @@ -3768,9 +3944,15 @@ static int dsa_user_fdb_event(struct net_device *dev,
>  	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
>  	switchdev_work->vid = fdb_info->vid;
>  	switchdev_work->host_addr = host_addr;
> +	switchdev_work->sync = host_addr && !fdb_info->is_local;

[Severity: Medium]
The commit message says "Addresses local to the bridge do not roam and
are still counted." Does that hold for bridge fdb replace?

In fdb_add_entry(), an existing entry on a foreign port can be turned
into a local br0 entry in place. fdb_update_dst() changes the
destination, NUD_PERMANENT sets BR_FDB_LOCAL, and only RTM_NEWNEIGH is
notified, with no deletion. The reverse change is also possible.

Here the local ADD has is_local set, so it gets sync = false. It goes
to the counted path, which never touches bridge_host_fdbs. Consider:

  bridge fdb replace ADDR dev br0 self local
  bridge fdb del ADDR dev br0 self

Only the counted reference is dropped. The records held from the
foreign phase keep ADDR on the CPU port until the ports leave the
bridge.

In the other direction, from local to static on a foreign port, the
counted reference from the local phase is never dropped.

Should this replace case be handled like the user port one?

> +	switchdev_work->replay = !!ctx;
>  
>  	dsa_schedule_work(&switchdev_work->work);
>  
> +	if (!host_addr && event == SWITCHDEV_FDB_ADD_TO_DEVICE)
> +		return dsa_user_bridge_host_fdb_release(dev, fdb_info->addr,
> +							fdb_info->vid);
> +

[Severity: Medium]
Can the release be skipped when a static entry moves onto a LAG?

Take a user-added entry on a LAG that the port offloads. orig_dev is
the LAG device, dsa_foreign_dev_check() returns false, and host_addr
stays false. dsa_user_fdb_event() then reaches this check, which comes
before the new call to dsa_user_bridge_host_fdb_release():

	if (dp->lag && !host_addr) {
		if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del)
			return -EOPNOTSUPP;

Drivers that offload LAGs but have no lag_fdb_add/lag_fdb_del, such
as mv88e6xxx and qca8k, return -EOPNOTSUPP there.

Suppose every user port of the bridge holds ADDR as a host address,
because the entry is on a foreign port such as a veth. Then run:

  bridge fdb replace ADDR dev bond0 master static

Here bond0 is an offloaded LAG of user ports. fdb_add_entry() only
notifies RTM_NEWNEIGH, so no DEL for the veth reaches DSA. The commit
message relies on the release for this replace case, but the release
is never scheduled. Nothing later re-evaluates the records either.
The address stays on the CPU port until the user ports leave the
bridge.

Dynamically learned ADDs on the LAG are not affected, since they take
the dsa_port_offloads_bridge_port() branch earlier. Should the release
be scheduled before the lag_fdb_add/lag_fdb_del check, so it does not
depend on whether the driver can install the entry itself?

[Severity: Medium]
On a bridge fdb replace from a foreign port onto a user port, can the
release erase the static entry that was just installed on the user
port?

The normal work queued above installs the entry on the user port
through ds->ops->port_fdb_add(). The release work runs after it on the
ordered dsa_owq. Dropping the last hold reaches ds->ops->port_fdb_del()
on the CPU port for the same MAC and VID.

On switches with a single unicast entry per {MAC, VID}, that delete
does not check which port owns the entry. b53_arl_op() does this for
unicast, with is_valid false on a delete:

	if (!is_multicast_ether_addr(addr)) {
		ent.port = port;
		ent.is_valid = is_valid;

mt7530_port_fdb_del() writes STATIC_EMP for the MAC/VID whatever the
owner:

	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_EMP);

ocelot_fdb_del() takes no port argument:

	return ocelot_mact_forget(ocelot, addr, vid);

Would the static entry then be missing from hardware, even though the
bridge already received SWITCHDEV_FDB_OFFLOADED for it?

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794134495.git.julius%40bairaktaris.de