Re: [PATCH net-next] net: dsa: b53: mirror BPDUs to CPU port 5 when port 8 is not a CPU port

[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: 0 · 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).

- [High] The new condition in b53_brcm_hdr_setup() calls
  `!dsa_is_cpu_port(ds, B53_CPU_PORT)` (port 8) without checking that…
- [Low] The commit fixes a user-visible bug but has no Fixes: tag.

--

net: dsa: b53: mirror BPDUs to CPU port 5 when port 8 is not a CPU port

On BCM531x5 switches where port 5 is the CPU port and port 8 is not,
this patch uses the ingress mirror to copy frames sent to the bridge
group address from the user ports to port 5. It also refuses tc
mirroring on those boards, because the switch has only one mirror.

> Signed-off-by: Joseph Telaak <joseph@telaak.dev>

[Severity: Low]
Should this commit carry a Fixes: tag?

The notes below the --- line name 8fab459e69ab ("net: dsa: b53: Enable
Broadcom tags for 531x5/539x families"), but those notes are dropped
when the patch is applied.

That commit added the dual-IMP selection for port 5 in
b53_brcm_hdr_setup():

	else if (port == 5)
		hdr_ctl |= GC_FRM_MGMT_PORT_M;

Without the tag, nothing in the applied commit records which kernels
lose BPDUs on these boards.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index a2a5754c413c6..6b33775c75985 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -797,6 +797,24 @@ void b53_brcm_hdr_setup(struct dsa_switch *ds, int port)
>  		hdr_ctl |= GC_FRM_MGMT_PORT_M;
>  	b53_write8(dev, B53_MGMT_PAGE, B53_GLOBAL_CONFIG, hdr_ctl);
>  
> +	/* With port 5 as the CPU port, a BCM531x5 runs in dual-IMP mode and
> +	 * traps BPDUs from LAN ports to port 8. When port 8 is not a CPU
> +	 * port, mirror them to port 5 instead. This takes the switch's only
> +	 * mirror, so tc mirroring is refused.
> +	 */
> +	if (tag_en && port == 5 && is531x5(dev) &&
> +	    !dsa_is_cpu_port(ds, B53_CPU_PORT)) {

[Severity: High]
Can dsa_is_cpu_port(ds, B53_CPU_PORT) dereference a NULL pointer when
port 8 has no dsa_port?

DSA creates dsa_port objects only for indices below ds->num_ports
(dsa_switch_touch_ports()). b53_switch_init() derives that count from
enabled_ports:

	dev->num_ports = fls(dev->enabled_ports);

	dev->ds->num_ports = min_t(unsigned int, dev->num_ports, DSA_MAX_PORTS);

When platform data is supplied (for example, b53_spi_probe() takes it
from spi->dev.platform_data), b53_switch_register() replaces the chip
table mask:

		dev->enabled_ports = dev->pdata->enabled_ports;

Take a BCM531x5 described this way, using port 5 as the CPU port and
with BIT(8) cleared in enabled_ports (for example 0x3f). That leaves
ds->num_ports at 6, and the call chain becomes:

b53_setup()
  b53_enable_cpu_port(dev, 5)
    b53_brcm_hdr_setup(ds, 5)
      dsa_is_cpu_port(ds, 8)
        dsa_to_port(ds, 8)    <- no index 8 on dst->ports, returns NULL
        ->type                <- NULL dereference

dsa_to_port() returns NULL when no port matches, and dsa_is_cpu_port()
uses the result without checking it. Before this patch, b53 did not look
up the dsa_port for B53_CPU_PORT.

Boards probed from DT get the chip table mask, which includes port 8,
so only boards using b53_platform_data can hit this.

Could a missing port 8 count as not a CPU port, so the BPDU mirror is
still installed? Something like:

	(B53_CPU_PORT >= ds->num_ports || !dsa_is_cpu_port(ds, B53_CPU_PORT))

> +		b53_write48(dev, B53_MGMT_PAGE, B53_IG_MIR_MAC,
> +			    ether_addr_to_u64(eth_stp_addr));

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-b53-bpdu-mirror-v1-1-f9f32e9d25db%40telaak.dev