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

[Vikas Gupta]

Hi,
  I'll send a v2 of this series with the patch order swapped.

Thanks,
Vikas

On Tue, Oct 13, 2026 at 11:14 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature