Re: [PATCH net 2/3] net: dsa: motorcomm: Rewrite FDB entry on port migration

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The fix is incomplete: an address that moves between a user port
  and the host (CPU port) still leaves a stale port bit in the hardware…
- [Low] yt921x_dsa_port_fdb_add() now calls dsa_bridge_ports(ds, bdev).

--

net: dsa: motorcomm: Rewrite FDB entry on port migration

This commit merges yt921x_fdb_join() and yt921x_fdb_leave() into a single
yt921x_fdb_update() helper that takes separate leave and join port masks.
yt921x_dsa_port_fdb_add() now passes the bridge port mask as the leave
mask, so ports left behind when an address migrates are dropped from the
hardware entry.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index f34f37b736249..d84e1f6dad843 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -877,10 +872,17 @@ yt921x_dsa_port_fdb_add(struct dsa_switch *ds, int port,
>  			const unsigned char *addr, u16 vid, struct dsa_db db)
>  {
>  	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	u32 bridge_ports_mask = 0;
> +	struct net_device *bdev;
>  	int res;
>  
> +	bdev = dsa_port_bridge_dev_get(dp);
> +	if (bdev)
> +		bridge_ports_mask = dsa_bridge_ports(ds, bdev);

[Severity: Low]
Is it safe to walk every port's dp->bridge here without rtnl?

dsa_bridge_ports() does a plain load of dp->bridge, then reads
dp->bridge->dev, for each user port via dsa_port_bridge_dev_get(). This
path runs from dsa_user_switchdev_event_work() on dsa_owq, and rtnl_lock is
not held there. Before this patch, the driver only called
dsa_bridge_ports() from the bridge_join and bridge_flags paths, which hold
rtnl.

The bridge leave path looks covered. dsa_port_pre_bridge_leave() flushes
dsa_owq under rtnl before dsa_port_bridge_destroy() runs. The rollback in
dsa_port_bridge_join() in net/dsa/port.c does not flush first, though:

out_rollback:
	dsa_port_bridge_destroy(dp, br);

Say FDB work queued by an earlier static fdb add on port 1 is still running
when a bridge join on port 2 fails. The work could load dp2->bridge just
before the kfree() and then read ->dev from freed memory.

The loaded value is only compared with bdev, so the practical impact is
small. Separately, bridge_join stores to dp->bridge on another port with a
plain write. That is a data race with this reader that KCSAN can see.

DSA core already does similar lockless cross-port reads in
dsa_foreign_dev_check() and dsa_tree_offloads_bridge_dev().

Could the port's own bridge come from the db argument (db.bridge) rather
than re-reading dp->bridge outside rtnl?

> +
>  	mutex_lock(&priv->reg_lock);
> -	res = yt921x_fdb_join(priv, addr, vid, BIT(port));
> +	res = yt921x_fdb_update(priv, addr, vid, bridge_ports_mask, BIT(port));

[Severity: High]
Does this still leave a stale port bit when an address moves between a user
port and the host?

The leave mask comes from the target port's own dp->bridge. This driver
sets ds->assisted_learning_on_cpu_port, so DSA installs host addresses by
calling .port_fdb_add() with port set to the CPU port. dp->bridge is only
set on user ports, so bdev is NULL and bridge_ports_mask is 0.

yt921x_fdb_update(priv, addr, vid, 0, BIT(cpu)) then keeps every existing
egress bit. The existing entry is already STATIC, so ctrl1 == val1 and the
full clear in yt921x_fdb_update() is skipped as well:

	ctrl1 = val1;
	if (join_ports_mask) {
		ctrl1 &= ~YT921X_FDB_IO1_STATUS_M;
		ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
		if (ctrl1 != val1)
			ctrl2 &= ~YT921X_FDB_IO2_EGR_PORTS_M;
	}
	ctrl2 |= YT921X_FDB_IO2_EGR_PORTS(join_ports_mask);

The result is still an OR-in, not a rewrite of the whole entry.

One way to reach this: take a static entry on swp1 and use bridge fdb
replace to point it at a foreign interface in the same bridge, or replace
it with a local/permanent entry. fdb_add_entry()->fdb_update_dst() only
sends RTM_NEWNEIGH, so swp1 never gets a SWITCHDEV_FDB_DEL_TO_DEVICE:

fdb_add_entry()
  fdb_update_dst()
  fdb_notify(br, fdb, RTM_NEWNEIGH, true)
    dsa_user_fdb_event()            /* host_addr = true */
      dsa_port_bridge_host_fdb_add()
        dsa_switch_host_fdb_add()
          dsa_port_do_fdb_add(cpu_dp)
            yt921x_dsa_port_fdb_add()   /* port == CPU port */

The swp1 bit then stays in a STATIC entry that DSA no longer tracks. A
later host fdb del clears only the CPU bit. Fast age through
yt921x_fdb_flush_port(..., false) does not flush static entries. Unicast
frames for that MAC keep going out of swp1.

The reverse direction seems to have the same problem.
dsa_switch_for_each_bridge_member() only iterates user ports, so the mask
from dsa_bridge_ports() never includes a CPU port. When a host address is
re-pointed to swp1, the old CPU port bit is never cleared, and frames for
that MAC also go to the CPU.

The commit message says:

  "rewrite the whole entry on add with the bridge port mask as the leave
  mask, so that ports left behind by the migration are dropped from the
  entry."

but egress bits for ports outside the bridge's user ports are kept.

The later patches in the series, up to "Use VID 0 as the unaware PVID",
don't appear to change yt921x_dsa_port_fdb_add() or yt921x_fdb_update().

For DSA_DB_BRIDGE host adds, db.bridge is a copy of *dp->bridge. Only .num
is zeroed, because fdb_isolation is not set.

Could db.bridge.dev be used to work out the bridge mask for CPU port
calls? Could the CPU ports also be added to the leave mask for user port
calls?

>  	mutex_unlock(&priv->reg_lock);
>  
>  	return res;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111234110.1449602-1-mmyangfl%40gmail.com