Re: [PATCH net 2/2] net: dsa: mxl862xx: reject bridge_id 0 from BRIDGE_ALLOC

[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: 2 · Low: 2

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] mxl862xx_allocate_bridge()
  (drivers/net/dsa/mxl862xx/mxl862xx.c:587-606) still checks the device-…
- [Medium] net: dsa: mxl862xx: off-by-one check order in
  mxl862xx_port_fdb_dump()
- [Low] The commit message does not mention one of the two hunks, and
  that hunk has nothing to do with bridge_id.
- [Low] The commit message has several gaps.

--

net: dsa: mxl862xx: reject bridge_id 0 from BRIDGE_ALLOC

This patch makes mxl862xx_allocate_bridge() return -EIO when the
BRIDGE_ALLOC response carries bridge_id 0. That value is reserved by the
API and collides with MXL862XX_DEFAULT_BRIDGE, which the CPU port uses.
The patch also rewrites the comment on the iteration bound in
mxl862xx_port_fdb_dump().

> mxl862xx_allocate_bridge() only checks the transport status (ret < 0)
> and returns le16_to_cpu(bridge_id) verbatim.

[Severity: Low]
I have a few small questions about the commit message.

The code being replaced checks "if (ret) return ret;", not ret < 0.
Should the description match the code?

Should this have a Fixes: tag? git blame shows all of
mxl862xx_allocate_bridge() came from:

Fixes: 340bdf984613 ("net: dsa: mxl862xx: implement bridge offloading")

The message doesn't mention that 0 is also the "not allocated" value
for priv->bridges[]:

mxl862xx_port_bridge_join() {
    ...
	if (!priv->bridges[bridge.num]) {
		ret = mxl862xx_allocate_bridge(priv);
    ...
}

mxl862xx_get_fid() {
    ...
	case DSA_DB_BRIDGE:
		if (!priv->bridges[db.bridge.num])
			return -ENOENT;
    ...
}

So with an id of 0, the bridge also looked unallocated. Each later join
allocated it again, and bridge FDB and MDB operations failed with -ENOENT.
Would it be worth describing this as well?

Firmware that follows the API never returns 0, so this change guards
against firmware that breaks the API. Could the message say so? It would
help with net vs net-next and stable decisions.

The Reported-by: trailer has the author's own address and there's no
Closes: link. Is that intended?

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> index 12cd40756a0c9..dbe20014d090e 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
> @@ -593,7 +593,16 @@ static int mxl862xx_allocate_bridge(struct mxl862xx_priv *priv)
>  	if (ret)
>  		return ret;
>  
> -	return le16_to_cpu(br_alloc.bridge_id);
> +	/* Per the API spec, id 0 is not used for historic reasons. Accepting
> +	 * it verbatim collides with MXL862XX_DEFAULT_BRIDGE (the CPU port),
> +	 * silently merging the port's FID with the CPU port's filtering
> +	 * domain.
> +	 */
> +	ret = le16_to_cpu(br_alloc.bridge_id);
> +	if (ret == 0)
> +		return -EIO;
> +
> +	return ret;
>  }

[Severity: Medium]
Is rejecting only 0 enough to cover what the commit message describes?
Every other u16 value is still returned as a valid FID.

The kernel-doc for struct mxl862xx_bridge_alloc in mxl862xx-api.h says:

 * @bridge_id: If the bridge allocation is successful, a valid ID will be
 *             returned in this field. Otherwise, INVALID_HANDLE is
 *             returned.

The driver doesn't define or check INVALID_HANDLE. If it is ~0, as in
the MaxLinear GSW API, 65535 would be accepted as a FID.

The commit message also says "duplicate non-zero ids returned for two
ports collapse their per-port FID isolation". However, nothing compares
the returned id with priv->ports[*].fid or priv->bridges[*], so
duplicates still get through.

There's also no upper bound check against the hardware FID range.
Commit 340bdf984613 says the switch supports at most 63 bridges.

Both callers only check for ret < 0 and then use the value:

mxl862xx_port_setup()
    priv->ports[port].fid = ret;
    -> mxl862xx_bridge_config_fwd(), mxl862xx_set_bridge_port()

mxl862xx_port_bridge_join()
    priv->bridges[bridge.num] = ret;
    -> mxl862xx_sync_bridge_members(), mxl862xx_get_fid(), BRIDGE_FREE

mxl862xx_port_fdb_del() matches an entry only by FID, VID and MAC. So if
two domains get the same FID, deleting an FDB entry in one domain can
remove an entry belonging to the other.

Should this check also reject INVALID_HANDLE, ids outside the hardware
range, and ids that are already in use?

[ ... ]

> @@ -1513,9 +1522,11 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
>  		if (ret)
>  			return ret;
>  
> -		/* The MAC table capacity is fixed by the device. A walk longer
> -		 * than that means the device (or its firmware) is not
> -		 * responding correctly; bail out instead of looping forever
> +		/* The exact MAC table capacity is not part of the public
> +		 * driver API. Use a conservative upper bound: 16k entries is
> +		 * well above any documented table size for this family, while
> +		 * a device that stopped responding correctly (e.g. firmware
> +		 * crash mid-walk) would otherwise keep us looping forever
>  		 * with RTNL held.
>  		 */
>  		if (count++ >= MXL862XX_MAC_TABLE_FDB_SIZE) {

[Severity: Medium]
The previous patch introduced this, not this one, but this hunk
rewrites the comment above it anyway. Is the bound checked in the
right place?

In mxl862xx_port_fdb_dump() the count check runs before the
param.last test. The final MXL862XX_MAC_TABLEENTRYREAD response only
sets last and carries no entry, but it is still counted against
MXL862XX_MAC_TABLE_FDB_SIZE.

With N valid entries the loop does N + 1 reads. On the last read,
count is already N. If the table holds exactly
MXL862XX_MAC_TABLE_FDB_SIZE entries, the dump returns -EIO and logs
"FDB dump exceeded table capacity". The firmware ended the walk
correctly, yet bridge fdb show fails on a full table.

The macro name and the error message both call 16384 the table
capacity. A full table is exactly the case the bound is meant to
allow. Should the param.last test be moved above the count check?

[Severity: Low]
This isn't a bug, but this hunk has nothing to do with bridge_id, and the
commit message doesn't mention it. It rewrites the comment that the
previous patch in the series added ("net: dsa: mxl862xx: bound FDB dump
walk by table capacity").

Should it be squashed into that patch instead?

The new comment says 16384 is not the table capacity, but the code
around it still says it is:

#define MXL862XX_MAC_TABLE_FDB_SIZE 16384
...
			dev_err(ds->dev, "FDB dump exceeded table capacity\n");

The previous patch's subject also says "by table capacity". Should the
macro name and the error message be changed to match the new comment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_A56873DA4A296F5D794B6A5989A180A21C05%40qq.com