Re: [PATCH net v2 2/5] net: dsa: b53: fix the SRAB grant timeout check

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The grant poll loop in b53_srab_request_grant()
  (drivers/net/dsa/b53/b53_srab.c, lines 102-109) wastes its final sleep.

Pre-existing issues:
- [Medium] Uninitialized stack values are used on the grant-timeout path
  that this patch makes reachable.

--

net: dsa: b53: fix the SRAB grant timeout check

This patch changes the timeout check in b53_srab_request_grant() from
i == 5 to i == 20 so that it matches the 20 iteration poll loop. A grant
that never arrives now returns -EIO, where before the register access
went ahead without ownership of the switch resources.

> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index 83a03fca142a0..f4689078ccd51 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -105,7 +105,7 @@ static int b53_srab_request_grant(struct b53_device *dev)
>  			break;
>  		usleep_range(10, 100);
>  	}
> -	if (WARN_ON(i == 5))
> +	if (WARN_ON(i == 20))
>  		return -EIO;

[Severity: Medium]
This is a pre-existing issue, but this patch makes the grant timeout
path reachable. Several callers then use uninitialized values.

When b53_srab_request_grant() returns -EIO, b53_srab_read8() (and the
read16/read32/read48/read64 variants) skip the *val assignment:

drivers/net/dsa/b53/b53_srab.c:b53_srab_read8() {
	ret = b53_srab_request_grant(dev);
	if (ret)
		goto err;
	...
	*val = readl(regs + B53_SRAB_RD_L) & 0xff;

err:
	b53_srab_release_grant(dev);
	...
}

The b53_build_op() wrappers in b53_priv.h pass the error through and do
not initialize *val. Many callers in b53_common.c ignore the return
value. One example is b53_get_ethtool_stats():

		} else {
			u32 val32;

			b53_read32(dev, B53_MIB_PAGE(port), s->offset,
				   &val32);
			val = val32;
		}
		data[i] = (u64)val;

Can this copy uninitialized kernel stack data into the ETHTOOL_GSTATS
buffer that is returned to userspace?

Other callers follow the same pattern:

  b53_eee_enable_set() writes an uninitialized u16 reg back to
  B53_EEE_EN_CTRL with b53_write16().

  b53_enable_vlan() read-modify-writes the uninitialized mgmt, vc0, vc1
  and vc5 into the switch mode and VLAN control registers.

  b53_arl_search_read_89() (and the _25/_63xx/_95 variants) decode the
  uninitialized mac_vid and fwd_entry into a b53_arl_entry.
  b53_fdb_dump() then passes that entry to b53_fdb_copy() and the FDB
  dump callback.

Before this patch, a grant timeout fell through to b53_srab_op() and
*val got written. The same uninitialized use could already happen
through the GORDYN timeout in b53_srab_op(). That path also returns
-EIO and skips the assignment.

Should the SRAB read accessors zero *val on error? The other option is
for these callers to check the b53_read*() return value.

[Severity: Low]
Can this reject a grant that arrives during the last sleep?

On the i == 19 iteration, the loop reads B53_SRAB_CTRLS and sees no
RCAGNT. It then calls usleep_range(10, 100) and exits with i == 20,
without reading CTRLS again:

	for (i = 0; i < 20; i++) {
		ctrls = readl(regs + B53_SRAB_CTRLS);
		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
			break;
		usleep_range(10, 100);
	}

Suppose the arbiter asserts RCAGNT during that last sleep. The function
now WARNs and returns -EIO. The caller's err: path then calls
b53_srab_release_grant() and clears RCAREQ, even though access was
granted. In practice the wait covers 19 polls, not 20.

Before this patch the timeout check never fired, so this had no effect.

Would readl_poll_timeout() avoid this? A final read of CTRLS after the
loop would also work.

>  
>  	return 0;

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