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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/30/26 01:25, netdev-bot+sashiko@kernel.org wrote:
> 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?

Yes, in theory. Not all supported switches are reset during setup. I 
will explicitly clear capture control and both source-control registers 
during b53 and bcm_sf2 setup, checking each write.


> 
> 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?

Agreed. This predates the patch. I will include the existing "net: dsa:
b53: reference count shared mirror source bits" fix from my local 
b53-fixes. This preserves support for multiple rules sharing one source 
and destination. The destination check remains ahead of reference 
increments, and the first reference is recorded only after both hardware 
writes succeed.

> 
> 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?

Agreed, also pre-existing. I will propagate both write errors and 
program the capture destination before enabling the new source. This 
avoids adding a source before a potentially failing destination write.

I will also base destination conflicts on the two source masks, rather
than MIRROR_EN alone. An unsuccessful final capture-control clear then
does not block a new destination when no sources remain; conversely,
remaining source bits prevent changing the destination even if MIRROR_EN
is clear.

> 
> 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?

Yes. The statement about propagating a failed capture-control read 
referred to the newly added read, but the other unchecked reads are real 
bugs. I will check all reads and writes in both helpers. Since 
port_mirror_del returns void, deletion errors can only be logged; a bus 
failure can still leave a source enabled, but subsequent adds will 
respect that source's
destination.

> 
>>   	/* 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?
> 

Agreed. Clearing CAP_PORT_MASK is cleanup of the inactive configuration;
it is not required to allow a subsequent destination change. I will
correct the description.

pw-bot: cr
-- 
Florian