Re: [PATCH net 2/2] bnge: cap RSS contexts and RX rings to the RSS table size

[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 commit message says the overflow happens with "577 or more RX
  rings".
- [Low] The commit message says the pad computation "in
  bnge_set_dflt_rss_indir_tbl() and bnge_modify_rss() underflows into a…

--

bnge: cap RSS contexts and RX rings to the RSS table size

This patch caps the value returned by bnge_cal_nr_rss_ctxs() at
BNGE_RSS_TABLE_MAX_TBL. It also limits max_rx in _bnge_get_max_rings() to
BNGE_MAX_RSS_TABLE_ENTRIES, so the RSS context count and the indirection
table size fit the arrays, which hold 8 contexts and 512 entries.

> bnge_cal_nr_rss_ctxs() has no upper bound. The VNIC holds at most
> BNGE_RSS_TABLE_MAX_TBL (8) RSS contexts, in fw_rss_cos_lb_ctx[] and in
> a 512-entry indirection table and DMA buffer. With 577 or more RX rings
> the function returns more than 8, so bnge_setup_vnic() writes past

[Severity: Low]
Is 577 the right threshold here? Here is bnge_adjust_pow_two() in
bnge_resc.h:

    u32 blks = total_ent / ent_per_blk;

    if (!blks)
        return 1;

    return roundup_pow_of_two(blks + 1);

With rx_rings = 512, bnge_cal_nr_rss_ctxs() passes 511, so blks = 7 and
the result is 8 contexts.

With rx_rings = 513 it passes 512, so blks = 8 and roundup_pow_of_two(9)
returns 16 contexts.

So the overflows described here seem to start at 513 RX rings. 577 is
one of the values in the 513..1024 range that also give 16. The code
change caps max_rx at 512, so it covers that whole range. Only the number
in the message looks wrong.

Could the message say 513 instead of 577?

> vnic->fw_rss_cos_lb_ctx[], bnge_set_dflt_rss_indir_tbl() and
> bnge_fill_hw_rss_tbl() write past the indirection table and its DMA
> buffer, and the pad computation in bnge_set_dflt_rss_indir_tbl() and
> bnge_modify_rss() underflows into a huge memset().

[Severity: Low]
Where is bnge_modify_rss() defined? I couldn't find a function by that
name under drivers/net/ethernet/broadcom/. bnge also doesn't seem to have
a set_rxfh or set_channels path where one would live.

The only pad underflow site in bnge seems to be in
bnge_set_dflt_rss_indir_tbl():

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

Was this reference carried over from bnxt? Should the message list only
the sites that exist in bnge, i.e. bnge_setup_vnic(),
bnge_fill_hw_rss_tbl() and bnge_set_dflt_rss_indir_tbl()?

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