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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 (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] Only one capture-control read is checked; the other register
  reads in the mirror helpers still ignore errors.
- [Medium] The new check in b53_mirror_add()
  (drivers/net/dsa/b53/b53_common.c:2553-2558) treats MIRROR_EN and…
- [Medium] b53_mirror_add() ignores the results of both hardware writes
  and always returns 0: the source-mask write `b53_write16(dev,…
- [Low] The commit message justifies the b53_mirror_del() hunk with
  "Clear the capture-port field when the final source is removed so a…

Pre-existing issues:
- [Medium] b53_mirror_add() does not reject a second mirror rule for the
  same source port and direction.

--

net: dsa: b53: reject conflicting mirror destinations

This commit makes b53_mirror_add() read B53_MIR_CAP_CTL before it touches
the source masks. If mirroring is already enabled toward a different
capture port, the new rule is rejected with -EOPNOTSUPP and an extack
message. b53_mirror_del() now clears the whole CAP_PORT_MASK field along
with MIRROR_EN once both source masks are empty.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0871a10d8cbc7..b8a7b380bac87 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]
This check uses only MIRROR_EN and CAP_PORT_MASK in B53_MIR_CAP_CTL. Can
that register hold stale state that this kernel never programmed?

B53_MIR_CAP_CTL, B53_IG_MIR_CTL and B53_EG_MIR_CTL are only accessed in
b53_mirror_add() and b53_mirror_del(). Neither b53_setup()->b53_reset_switch()
nor the bcm_sf2 setup paths initialize them.

b53_switch_reset() only does a soft reset for is539x() and
BCM58XX/BCM583XX, plus a reset GPIO when one is wired:

    b53_switch_reset_gpio(dev);

    if (is539x(dev)) {
        b53_write8(dev, B53_CTRL_PAGE, B53_SOFTRESET, 0x83);
        ...

On chips like BCM53115/53125/53128, BCM5325/5365, BCM63xx or BCM5301x
without a reset GPIO, mirror state left by a bootloader or a previous
kernel survives. kexec is one example: dsa_switch_shutdown() never calls
port_mirror_del, so the old kernel's tc mirror rules stay in hardware.

If MIRROR_EN is left set, every rule to a different port fails with "Only
one mirroring destination is supported", even though no mirror rule is
installed.

Adding and then deleting a rule to the stale destination may not clear
it either. b53_mirror_del() clears only BIT(port), and it clears MIRROR_EN
only when both source masks are empty. Leftover source bits for other
ports keep MIRROR_EN set.

Before this patch the destination was simply overwritten. Should the
mirror registers be reset during setup, or should the driver track
installed rules in software?

[ ... ]

> @@ -2554,11 +2566,10 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  	reg |= BIT(port);

[Severity: Medium]
This is a pre-existing issue and wasn't introduced by this patch.
b53_mirror_add() accepts a second rule for the same source port and
direction. What happens to the remaining rule when one of the duplicates
is deleted?

dsa_user_add_cls_matchall_mirred() passes every matchall mirror rule to
the driver without a duplicate check. Each tc prio gets its own matchall
tcf_proto, so two rules A and A' (port 1 ingress to port 2) can share one
hardware bit.

Deleting A clears BIT(1) in B53_IG_MIR_CTL. If no other source remains, it
also clears MIRROR_EN and CAP_PORT_MASK. A' is still reported as offloaded
but does nothing in hardware.

A later rule C (port 1 ingress to port 4) then passes the new check, and
A' traffic goes to port 4.

mt753x_port_mirror_add() and qca8k_port_mirror_add() reject duplicates
using software masks:

    if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
        return -EEXIST;

The new check assumes MIRROR_EN reflects the live rule set. Would b53 need
similar tracking?

>  	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: Medium]
Both b53_write16() results here are ignored, and 0 is always returned.
Can a failed write leave the hardware out of sync with what DSA has
recorded?

b53_mdio_write16() can fail before the target register is written, either
from mdiobus_write_nested() or with -EIO from b53_mdio_op(). SPI and SRAB
writes can fail too.

Suppose the source-mask write succeeds but the B53_MIR_CAP_CTL write
fails. MIRROR_EN stays clear, but dsa_user_add_cls_matchall_mirred() still
adds the rule to mall_tc_list because 0 was returned. A later rule to
another destination passes the new check and redirects the leftover
source. That is the redirect this patch is meant to prevent.

The final B53_MIR_CAP_CTL write in b53_mirror_del() is also unchecked. If
it fails, MIRROR_EN and the old capture port stay set with no sources, and
the new check rejects every other destination.

Should these writes be checked, with the source bit rolled back if the
capture-control write fails?

>  }

[ ... ]

> @@ -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);

[Severity: Medium]
The commit message says the patch will "propagate a failed
capture-control read rather than making the decision from uninitialized
data". This capture-control read is still unchecked, though. Is that
intended?

The b53 backends leave *val unchanged on error. b53_mdio_read16() returns
before assigning it:

    ret = b53_mdio_op(dev, page, reg, REG_MII_ADDR_READ);
    if (ret)
        return ret;

b53_spi_read16() only assigns on success.

reg is reused here. If this read fails, reg still holds the other
direction's B53_IG_MIR_CTL or B53_EG_MIR_CTL word, and that word is written
to B53_MIR_CAP_CTL unconditionally. The source bits land in CAP_PORT_MASK,
and the MIRROR_FILTER bits land in BLK_NOT_MIR/MIRROR_EN.

That usually clears MIRROR_EN while rules remain. A later rule to a
different destination then passes the new check in b53_mirror_add() and
silently redirects the remaining sources.

b53_mirror_add() has the same problem, and there reg is uninitialized:

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

If that read fails, stack garbage ORed with BIT(port) goes into the
source-mask register. That can mirror arbitrary ports or set
DIV_EN/MIRROR_FILTER. MIRROR_EN is then set and 0 is returned.

Later deletes only clear BIT(port). The stray bits keep MIRROR_EN set, so
the new check rejects every other destination.

The earlier reads in b53_mirror_del() that set loc_disable and
other_loc_disable are unchecked as well. Should all of these reads be
checked?

>  	/* 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: Low]
This isn't a bug, but the commit message says:

    Clear the capture-port field when the final source is removed so a later
    rule can select a new destination.

Is this hunk needed for that? The new check in b53_mirror_add() only
applies when MIRROR_EN is set, and b53_mirror_add() always rewrites the
field:

    cap_ctl &= ~CAP_PORT_MASK;
    cap_ctl |= mirror->to_local_port;

The old reg &= ~mirror->to_local_port already cleared the field whenever
it held to_local_port. Could the commit message call this a cleanup
rather than a requirement?

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