Re: [PATCH net 1/2] bnge: fix RSS table sizing for large RX ring counts

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

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] This commit opens a heap and DMA out-of-bounds write window for
  513-576 RX rings.
- [Medium] The commit message is wrong about the completion ring.

--

bnge: fix RSS table sizing for large RX ring counts

This changes bnge_adjust_pow_two() to always add one block before rounding
up to a power of two. RX ring counts whose quotient is already a power of
two now get an RSS indirection table that covers all rings. The same
helper also sizes the RX, TX, agg and completion ring pages.

> Always add one block before rounding up. The same helper sizes the ring
> pages; their count only changes when the number of entries divided by
> the entries per page is a power of two of 2 or more, which does not
> happen with the default ring sizes.

[Severity: Medium]
Is this accurate for the completion ring? It holds for the RX, TX and agg
rings at the default depth of 511. The completion ring, though, is sized
from more than the default depths:

bnge_netdev.c:bnge_set_ring_params() {
    ...
	max_rx_cmpl = bn->rx_ring_size;

	if (bn->priv_flags & BNGE_NET_EN_TPA)
		max_rx_cmpl += bd->max_tpa_v2;
	ring_size = max_rx_cmpl * 2 + agg_ring_size + bn->tx_ring_size;
	bn->cp_ring_size = ring_size;

	bn->cp_nr_pages = bnge_adjust_pow_two(ring_size, CP_DESC_CNT);
    ...
}

Take BNGE_PAGE_SIZE of 8192 (for example arm64 with 16K pages), TPA off
and default 511/511 rings. Then ring_size is 2 * 511 + 511 = 1533 and
CP_DESC_CNT is 512, so the quotient is 2. The old code returned 2 pages,
which gives a cp_bit of 1024 (fewer slots than cp_ring_size). The new code
returns 4.

With 4K pages and TPA on, agg_ring_size is 2044 and ring_size is
3577 + 2 * max_tpa_v2. max_tpa_v2 comes from firmware max_aggs_supported.
For values of 260..387, the quotient over a CP_DESC_CNT of 256 is 16, and
the page count goes from 16 to 32.

Could the commit message mention this change to completion ring sizing?

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_resc.h b/drivers/net/ethernet/broadcom/bnge/bnge_resc.h
> index b62a634669f62..c0b6992d7f828 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_resc.h
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_resc.h
> @@ -81,13 +81,10 @@ bnge_adjust_pow_two(u32 total_ent, u16 ent_per_blk)
>  {
>  	u32 blks = total_ent / ent_per_blk;
>  
> -	if (blks == 0 || blks == 1)
> -		return ++blks;
> +	if (!blks)
> +		return 1;
>  
> -	if (!is_power_of_2(blks))
> -		blks = roundup_pow_of_two(blks);
> -
> -	return blks;
> +	return roundup_pow_of_two(blks + 1);

[Severity: High]
For 513..576 RX rings, bnge_cal_nr_rss_ctxs() calls
bnge_adjust_pow_two(rx_rings - 1, 64), which gives a quotient of 8. The
old code returned 8, but this now returns 16. Nothing clamps the result
to BNGE_RSS_TABLE_MAX_TBL (8):

bnge_resc.c:bnge_cal_nr_rss_ctxs() {
	...
	return bnge_adjust_pow_two(rx_rings - 1,
				   BNGE_RSS_TABLE_ENTRIES);
}

Can this overflow bd->rss_indir_tbl[]? bnge_alloc_rss_indir_tbl()
allocates BNGE_MAX_RSS_TABLE_ENTRIES (512) entries. On the first
reservation, bnge_reserve_rings()->bnge_set_dflt_rss_indir_tbl() gets a
max_entries of 16 * 64 = 1024:

bnge_resc.c:bnge_set_dflt_rss_indir_tbl() {
	...
	for (i = 0; i < max_entries; i++)
		rss_indir_tbl[i] = ethtool_rxfh_indir_default(i,
							      bd->rx_nr_rings);

	pad = bd->rss_indir_tbl_entries - max_entries;
	if (pad)
		memset(&rss_indir_tbl[i], 0, pad * sizeof(*rss_indir_tbl));
}

The loop writes 512 entries past the end. Then the u16 pad wraps to
512 - 1024 = 65024, so the memset() clears about 254 KB beyond that.

The other RSS consumers also look sized for at most 8 contexts:

- bnge_setup_vnic() calls bnge_hwrm_vnic_ctx_alloc(bd, vnic, i) for
  i < 16. That stores into vnic->fw_rss_cos_lb_ctx[], which has only
  BNGE_MAX_CTX_PER_VNIC (8) entries.

- bnge_fill_hw_rss_tbl() uses a tbl_size of 1024. It reads
  rss_indir_tbl[] past 512 entries and writes 4096 bytes into
  vnic->rss_table. bnge_alloc_vnic_attributes() sizes that buffer from
  BNGE_MAX_RSS_TABLE_SIZE (2048 bytes plus the hash key).

- bnge_hwrm_vnic_set_rss() reads fw_rss_cos_lb_ctx[8..15]. It also moves
  ring_grp_tbl_addr past the end of the coherent allocation, so firmware
  would DMA read outside the buffer.

Configurations with 577 or more RX rings already overflowed before this
patch, but this patch lowers the threshold to 513. Hitting it needs at
least 513 default RSS queues from bnge_net_init_dflt_rings() and firmware
that grants that many rings.

The next patch in the series, "bnge: cap RSS contexts and RX rings to the
RSS table size", closes this. It adds a min_t() against
BNGE_RSS_TABLE_MAX_TBL in bnge_cal_nr_rss_ctxs() and caps *max_rx at
BNGE_MAX_RSS_TABLE_ENTRIES in _bnge_get_max_rings().

Could that cap be folded into this patch, or moved ahead of it, so this
commit can be bisected or backported safely on its own?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012054325.1782819-1-vikas.gupta%40broadcom.com