Re: [PATCH net v2 3/5] net: dsa: b53: clear stale state when creating an ARL entry

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] In b53_arl_op() (drivers/net/dsa/b53/b53_common.c), the patch
  changes the result of deleting an ARL entry that is not present (op ==…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but in
  b53_arl_op() (drivers/net/dsa/b53/b53_common.c) the unicast delete…

--

net: dsa: b53: clear stale state when creating an ARL entry

This patch clears the b53_arl_entry in b53_arl_op() before creating a new
ARL entry. As a result, a multicast add no longer ORs the port into an
unrelated bin's forwarding bitmap. It also makes b53_arl_op() return early,
without writing anything, when asked to delete an entry that is not
present.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 559d4b360c828..c38e8c5838bd8 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1990,11 +1990,15 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
>  	case -ENOSPC:
>  		dev_dbg(dev->dev, "{%pM,%.4d} no space left in ARL\n",
>  			addr, vid);
> -		return is_valid ? ret : 0;
> +		return is_valid ? ret : -ENOENT;
>  	case -ENOENT:
> +		if (!is_valid)
> +			return ret;
> +

[Severity: Medium]
Is it intended that deleting an absent entry now fails with -ENOENT?

Before this patch, the -ENOSPC delete case returned 0. The -ENOENT delete
case returned the result of b53_arl_rw_op(), which is normally 0.

The commit message only says the function will "return without writing
anything". In practice both paths now return -ENOENT to b53_fdb_del() and
b53_mdb_del(), and from there to the DSA core for both b53 and bcm_sf2.

On BCM5325/BCM5365, b53_mdb_add() never programs anything:

	if (is5325(priv) || is5365(priv))
		return -EOPNOTSUPP;

b53_mdb_del() has no matching check. Every RTM_DELMDB that comes through
br_switchdev_mdb_notify() -> switchdev_port_obj_del() will now hit this
path and log:

	if (ret)
		dev_err(ds->dev, "failed to delete MDB entry\n");

switchdev_port_obj_del_deferred() then logs a second error, because the
code is not -EOPNOTSUPP. Ordinary IGMP/MLD group leaves or timeouts on a
bridged user port would produce these messages.

The same thing happens when deleting an fdb/mdb entry whose earlier add
failed with -ENOSPC.

CPU and DSA ports see a further effect. In net/dsa/switch.c,
dsa_port_do_fdb_del() keeps the tracking record when the driver returns an
error, and dsa_port_do_mdb_del() does the same:

	err = ds->ops->port_fdb_del(ds, port, addr, vid, db);
	trace_dsa_fdb_del_hw(dp, addr, vid, &db, err);
	if (err) {
		refcount_set(&a->refcount, 1);
		goto out;
	}

A later add of the same address finds that record and calls
refcount_inc(). It never calls port_fdb_add()/port_mdb_add() again, so the
ARL entry stays missing until teardown. Before this patch, the delete
returned 0, the record was freed, and the next add reprogrammed the
hardware.

There are two ways the hardware can be missing an entry that DSA still
tracks:

  - bcm_sf2_sw_resume() resets the switch via bcm_sf2_sw_rst() and does
    not replay the host FDB/MDB lists.
  - b53 has no FDB isolation. A host entry and a user-port static entry
    with the same MAC/VID share one ARL slot, so deleting the user-port
    entry invalidates both.

Documentation/networking/dsa/dsa.rst says port_fdb_del/port_mdb_del remove
the address "if it was mapped". That reads as if a missing entry is not an
error.

Would returning 0 here (perhaps with a dev_dbg()), while still skipping the
write, keep the fix without these side effects?

>  		/* We could not find a matching MAC, so reset to a new entry */
>  		dev_dbg(dev->dev, "{%pM,%.4d} not found, using idx: %d\n",
>  			addr, vid, idx);
> +		memset(&ent, 0, sizeof(ent));
>  		break;
>  	default:
>  		dev_dbg(dev->dev, "{%pM,%.4d} found, using idx: %d\n",

[Severity: Medium]
This isn't a bug introduced by this patch, but should the unicast delete in
the found case check that the entry belongs to the requesting port?

After the switch statement, b53_arl_op() does:

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

It then writes the bin back as invalid. It never compares the ent.port read
from hardware against port.

dsa.rst says port_fdb_del should delete the address "if it was mapped into
this port forwarding database".

b53 does not set ds->fdb_isolation, so dsa_port_fdb_del() forces
info.db.bridge.num to 0 and the driver ignores the dsa_db. Suppose the same
MAC/VID is added on port A and then on port B. This can happen with two
VLAN-unaware bridges using vid 0, or with a host FDB entry on the CPU port
plus a user-port static entry.

The single ARL slot then points at B. A later delete on port A removes B's
entry, while the bridge and DSA still believe it is installed.

This patch only reworks the absent-entry branches, so the found-entry path
is unchanged.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028212310.3847553-1-florian.fainelli%40broadcom.com