Re: [PATCH net v3 4/8] net: dsa: b53: reject conflicting mirror destinations

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (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] At this commit, the new conflict check in b53_mirror_add()
  (drivers/net/dsa/b53/b53_common.c:2553-2557) decides whether rules…
- [Low] The commit message says the patch propagates a failed capture-
  control read, but only the B53_MIR_CAP_CTL read in b53_mirror_add()…

Pre-existing issues:
- [Medium] Each per-port, per-direction mirror source bit is shared by
  every tc matchall mirror rule on that port and direction, but it is…

--

net: dsa: b53: reject conflicting mirror destinations

With this patch, b53_mirror_add() reads B53_MIR_CAP_CTL first. While
MIRROR_EN is set, it rejects a new mirror rule whose destination differs
from the one already programmed. b53_mirror_del() now clears the whole
CAP_PORT_MASK field along with MIRROR_EN when the last source is removed.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index c38e8c5838bd8..378f47e7250cd 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2543,7 +2543,19 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  		   struct netlink_ext_ack *extack)
>  {
>  	struct b53_device *dev = ds->priv;
> -	u16 reg, loc;
> +	u16 reg, loc, cap_ctl;
> +	int ret;
> +
> +	ret = b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &cap_ctl);
> +	if (ret)
> +		return ret;
> +
> +	if ((cap_ctl & MIRROR_EN) &&
> +	    (cap_ctl & CAP_PORT_MASK) != mirror->to_local_port) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Only one mirroring destination is supported");
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Can leftover hardware state make this check reject a valid first rule?

The check only looks at MIRROR_EN and CAP_PORT_MASK as read back from
B53_MIR_CAP_CTL. It doesn't look at B53_IG_MIR_CTL or B53_EG_MIR_CTL.

At this commit, nothing in the setup path writes any of these three
registers. The only writers are b53_mirror_add() and b53_mirror_del().
Also, b53_switch_reset() doesn't soft-reset every chip.

Suppose a bootloader, a previous kernel (kexec or warm reboot) or an
earlier driver bind left MIRROR_EN set with capture port X. The first tc
mirror rule to any port other than X would then fail with -EOPNOTSUPP and
"Only one mirroring destination is supported", even though no software
rule exists. Before this patch, the add path just overwrote the stale
destination.

The reverse case also seems possible. Stale source bits with MIRROR_EN
clear pass this check, so those ports end up mirrored. The same stale bits
also stop b53_mirror_del() from ever clearing MIRROR_EN.

The later patch "net: dsa: b53: harden mirror register programming" in
this series seems to fix this. It adds b53_mirror_init(), which zeroes all
three registers from b53_setup() and bcm_sf2_sw_setup(). It also bases the
occupancy test on the source masks:

	if (((reg | other_reg) & MIRROR_MASK) &&
	    (cap_ctl & cap_mask) != cap_port) {

Could that be folded into this patch, so this commit doesn't regress on
its own?

>  
>  	if (ingress)
>  		loc = B53_IG_MIR_CTL;
> @@ -2554,11 +2566,10 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  	reg |= BIT(port);
>  	b53_write16(dev, B53_MGMT_PAGE, loc, reg);
>  
> -	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
> -	reg &= ~CAP_PORT_MASK;
> -	reg |= mirror->to_local_port;
> -	reg |= MIRROR_EN;
> -	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);
> +	cap_ctl &= ~CAP_PORT_MASK;
> +	cap_ctl |= mirror->to_local_port;
> +	cap_ctl |= MIRROR_EN;
> +	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, cap_ctl);
>  
>  	return 0;
>  }

[Severity: Low]
The commit message says the patch will "propagate a failed
capture-control read rather than making the decision from uninitialized
data". Are the other register accesses here still unchecked?

Just above this hunk, the source mask read is not checked:

	b53_read16(dev, B53_MGMT_PAGE, loc, &reg);
	reg |= BIT(port);
	b53_write16(dev, B53_MGMT_PAGE, loc, reg);

reg is an uninitialized stack variable. On failure, b53_mdio_read16() and
b53_spi_read16() return without writing *val. A failed read therefore
writes garbage back into B53_IG_MIR_CTL or B53_EG_MIR_CTL.

Both writes are unchecked too, and the function returns 0 either way. DSA
then keeps a mall entry that the hardware never accepted.

The source bit is enabled before B53_MIR_CAP_CTL is written. If the
capture-control write fails, MIRROR_EN can stay clear, and a later rule to
a different destination would pass the new guard.

b53_mirror_del() has the same pattern. If its B53_MIR_CAP_CTL read fails,
reg still holds the other-direction source mask, and that value is written
into the capture control register:

	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
	...
	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);

The later patch "net: dsa: b53: harden mirror register programming" in
this series seems to fix all of these. Every read and write is checked,
B53_MIR_CAP_CTL is programmed before the source bit is enabled, and the
delete path jumps to an error label instead of writing back garbage.

> @@ -2595,10 +2606,8 @@ void b53_mirror_del(struct dsa_switch *ds, int port,
>  
>  	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
>  	/* Both no longer have ports, let's disable mirroring */
> -	if (loc_disable && other_loc_disable) {
> -		reg &= ~MIRROR_EN;
> -		reg &= ~mirror->to_local_port;
> -	}
> +	if (loc_disable && other_loc_disable)
> +		reg &= ~(MIRROR_EN | CAP_PORT_MASK);

[Severity: Medium]
This isn't a bug introduced by this patch, but each per-port,
per-direction source bit is shared by every matchall mirror rule on that
port and direction, and it is not reference counted.

dsa_user_add_cls_matchall_mirred() in net/dsa/user.c allows several
identical rules, and each one is tracked as its own mall entry. Earlier in
b53_mirror_del(), deleting any one of them clears the shared bit:

	reg &= ~BIT(port);

With this patch, if that was the last bit, MIRROR_EN and CAP_PORT_MASK are
cleared as well. Does the remaining rule then stop mirroring in hardware
while it is still installed? And would the new guard in b53_mirror_add()
then accept a rule to a different destination?

The later patch "net: dsa: b53: reference count shared mirror source
bits" in this series seems to fix this. It adds mirror_count[2] to struct
b53_port, so the hardware is programmed only on the first reference and
cleared only when the last one goes away:

	if (--(*mirror_count))
		return;

>  	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);
>  }
>  EXPORT_SYMBOL(b53_mirror_del);

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