Re: [PATCH net-next] net: dsa: motorcomm: Add LAG FDB callbacks

[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: 1 · Low: 0

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 new yt921x_dsa_lag_fdb_add() and yt921x_dsa_lag_fdb_del() in
  drivers/net/dsa/motorcomm/chip.c write a one-time snapshot of the…
- [Medium] net: dsa: motorcomm: race on dp->lag in
  yt921x_dsa_lag_fdb_add() and del()

--

net: dsa: motorcomm: Add LAG FDB callbacks

This commit adds lag_fdb_add and lag_fdb_del callbacks to the yt921x
driver. Each one builds a bitmap of the current LAG member ports with
dsa_lag_foreach_port() and passes it to yt921x_fdb_join() or
yt921x_fdb_leave().

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 33e5fc127244d..f73eed06437b9 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -883,6 +883,52 @@ yt921x_dsa_port_fdb_add(struct dsa_switch *ds, int port,
>  	return res;
>  }
>  
> +static int
> +yt921x_dsa_lag_fdb_del(struct dsa_switch *ds, struct dsa_lag lag,
> +		       const unsigned char *addr, u16 vid, struct dsa_db db)
> +{

[ ... ]

> +	ports_mask = 0;
> +	dsa_lag_foreach_port(dp, ds->dst, &lag)
> +		ports_mask |= BIT(dp->index);
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_fdb_leave(priv, addr, vid, ports_mask);
> +	mutex_unlock(&priv->reg_lock);
> +
> +	return res;
> +}
> +
> +static int
> +yt921x_dsa_lag_fdb_add(struct dsa_switch *ds, struct dsa_lag lag,
> +		       const unsigned char *addr, u16 vid, struct dsa_db db)
> +{

[Severity: Medium]
Is it safe to walk the LAG membership here without rtnl_lock?

lag_fdb_add and lag_fdb_del are reached from
dsa_user_switchdev_event_work(). That runs on dsa_owq and does not hold
rtnl_lock. On that path, dsa_switch_do_lag_fdb_add() and
dsa_switch_do_lag_fdb_del() only take lag->fdb_lock before calling into
the driver. In both new callbacks the loop also runs before
priv->reg_lock is taken, so nothing in the driver serializes it either.

dsa_lag_foreach_port() does not only visit the members of this LAG. It
walks every port in ds->dst->ports and calls dsa_port_offloads_lag()
on each one, which goes through dsa_port_lag_dev_get():

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

At the same time, under rtnl_lock, dsa_port_lag_join() sets dp->lag
via dsa_port_lag_create() before DSA_NOTIFIER_LAG_JOIN is sent.
yt921x_dsa_port_lag_join() can then reject the port, for example when
there are more members than YT921X_LAG_PORT_NUM or the hash mode does
not match. In that case the err_lag_join rollback calls
dsa_port_lag_destroy(). That clears dp->lag and kfree()s the dsa_lag
once its refcount reaches zero. dsa_port_lag_leave() also calls
dsa_port_lag_destroy() before DSA_NOTIFIER_LAG_LEAVE.

So an FDB work item for one LAG can see another port's dp->lag as
non-NULL and then read ->dev after that dsa_lag has been freed, for
example when that port tried to create a new LAG and was refused. Even
when nothing is freed, the loop can include a port that the driver has
just refused, or a port that is halfway through leaving. That port then
ends up in YT921X_FDB_IO2_EGR_PORTS of a static entry.

Only CAP_NET_ADMIN can trigger the join and leave, so this is not a
security boundary. It can still crash the machine or misprogram the
FDB while a bridged bond is being reconfigured.

Could the driver keep its own record of LAG membership instead? It
would be updated in yt921x_dsa_port_lag_join() and
yt921x_dsa_port_lag_leave() under priv->reg_lock, and ports_mask would
be computed from it after reg_lock is taken here. That would also give
a natural place to reprogram existing LAG FDB entries when membership
changes. Or is there some DSA core serialization against dp->lag
changes on this path that I am missing?

[ ... ]

> +	ports_mask = 0;
> +	dsa_lag_foreach_port(dp, ds->dst, &lag)
> +		ports_mask |= BIT(dp->index);
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_fdb_join(priv, addr, vid, ports_mask);
> +	mutex_unlock(&priv->reg_lock);

[Severity: High]
What happens to this FDB entry if the LAG's membership changes after it
is installed?

The FDB destination is a plain physical port bitmap
(YT921X_FDB_IO2_EGR_PORTS_M is GENMASK(28, 18)). So ports_mask records
which ports were members when the call was made, and nothing updates it
afterwards.

DSA core refcounts LAG FDB entries per LAG. It only calls the driver on
the first add and on the last delete:

net/dsa/switch.c:dsa_switch_do_lag_fdb_add() {
    ...
	a = dsa_mac_addr_find(&lag->fdbs, addr, vid, db);
	if (a) {
		refcount_inc(&a->refcount);
		...
		goto out;
	}
    ...
}

net/dsa/switch.c:dsa_switch_do_lag_fdb_del() {
    ...
	if (!refcount_dec_and_test(&a->refcount)) {
		...
		goto out;
	}

	err = ds->ops->lag_fdb_del(ds, *lag, addr, vid, db);
    ...
}

yt921x_dsa_port_lag_join() and yt921x_dsa_port_lag_leave() only
reprogram the LAG group through yt921x_lag_set(). They don't touch FDB
entries.

Take bond0 = {swp1}, bridged, with "bridge fdb add X dev bond0 master
static". The hardware entry gets EGR_PORTS = {swp1}.

When swp2 joins bond0, the replayed add only bumps the refcount. The
entry stays {swp1}.

When swp1 leaves bond0, dsa_port_pre_lag_leave()->
dsa_port_pre_bridge_leave() replays the delete, and that only drops the
refcount. The entry still points at swp1. swp1 is now standalone and
yt921x_bridge_leave() has isolated it. Unicast to X is blackholed or
misdirected, and swp2 never gets this traffic.

On the final delete, yt921x_dsa_lag_fdb_del() rebuilds the mask from the
current members ({swp2}). Then yt921x_fdb_leave() does this:

	ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(ports_mask);
	if (ctrl2 == val2)
		return 0;
	if (!(ctrl2 & YT921X_FDB_IO2_EGR_PORTS_M)) {
		ctrl = YT921X_FDB_OP_OP_DEL | YT921X_FDB_OP_START;
		...
	}
	...
	return yt921x_fdb_add_index_in12(priv, index, ctrl1, ctrl2);

It either returns 0 without deleting anything, or rewrites the entry as
STATIC with the leftover bits. Doesn't that leave a STATIC hardware FDB
entry that DSA no longer tracks?

port_fast_age wouldn't remove it either, because
yt921x_fdb_flush_port(priv, port, false) leaves static entries alone. So
the entry would use an FDB slot and misforward X/vid until the chip is
reset.

Before this patch, lag_fdb_add and lag_fdb_del were NULL, so
dsa_user_fdb_event() returned -EOPNOTSUPP and this state could not
happen.

ocelot, the other lag_fdb_add implementer, keeps track of LAG FDBs and
reprograms them when membership changes, in ocelot_migrate_lag_fdbs().
Does this driver need something similar, called from
yt921x_dsa_port_lag_join() and yt921x_dsa_port_lag_leave()?

There is also a hardware question that can't be answered from the code.
When a unicast FDB lookup hits an entry with several egress ports, does
the ASIC pick one using the LAG hash, or does it send the frame to every
listed port? If it does the latter, every frame to X would be duplicated
across the LAG members even with no membership changes.

> +
> +	return res;
> +}

[ ... ]

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