Re: [net-next, v4 01/10] bnge: restructure VNIC and filter code

[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 does not describe everything the patch
  changes.

Pre-existing issues:
- [Medium] The RSS arrays are sized for at most 8 contexts, but the
  context count that indexes them has no upper bound.
- [Medium] bnge_cal_nr_rss_ctxs() returns too few RSS contexts for some
  ring counts, so some receive queues never appear in the default…
- [Medium] At this commit, a duplicate unicast MAC shares one refcounted
  filter, which causes a firmware filter leak and a double firmware free.

--

bnge: restructure VNIC and filter code

This patch moves the VNIC and L2 filter code out of bnge_netdev.c, and the
default RSS indirection table helper out of bnge_resc.c, into new
bnge_vnic.c/h and bnge_filter.c/h files. The commit message describes it
as a reorganization with no functional change, in preparation for
multi-VNIC, RSS context and NTUPLE filter support.

> Move VNIC and L2 filter code out of bnge_netdev.c into dedicated
> bnge_vnic.c/h and bnge_filter.c/h files in preparation for multi-VNIC,
> RSS context, and NTUPLE filter support.
>
> This is a code reorganization with no functional change, which helps
> centralize the functions into their respective modules.

[Severity: Low]
This isn't a bug, but does this description cover everything the patch
changes? Besides moving code out of bnge_netdev.c, the patch also:

- renumbers constants while turning them into enums: BNGE_FLTR_TYPE_L2
  goes from 2 to 1, BNGE_FLTR_FW_DELETED from 2 to 1,
  BNGE_VNIC_MCAST_FLAG from 4 to BIT(1) and BNGE_VNIC_UCAST_FLAG from 8
  to BIT(2)

- renames the struct bnge_filter_base members hash and list to hlist and
  list_node

- renames bnge_rss_ctxs_in_use() to bnge_get_total_rss_ctxs() and drops
  the rx_rings argument of bnge_get_total_vnics()

- narrows types: j in bnge_fill_hw_rss_tbl() goes from u32 to u16, the
  ring ids now go through a u16 temporary, and the loop counter in
  bnge_set_dflt_rss_indir_tbl() goes from int to u16

- makes bnge_set_dflt_rss_indir_tbl() and bnge_hwrm_vnic_rss_cfg()
  non-static, and moves bnge_set_dflt_rss_indir_tbl() out of
  bnge_resc.c, not bnge_netdev.c

All of these values seem to stay inside the driver, so runtime behaviour
looks unchanged. Even so, the removed and added lines can't be checked
against each other as a pure move.

Could the commit message list these changes? Another option is to split
the patch into a pure move followed by a cleanup.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> new file mode 100644
> index 0000000000000..a8bb441ebef9a
> --- /dev/null
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> @@ -0,0 +1,113 @@

[ ... ]

> +static struct bnge_l2_filter *bnge_lookup_l2_filter(struct bnge_net *bn,
> +						    struct bnge_l2_key *key,
> +						    u32 idx)
> +{
> +	struct bnge_l2_filter *fltr;
> +
> +	rcu_read_lock();
> +	fltr = __bnge_lookup_l2_filter(bn, key, idx);
> +	if (fltr)
> +		refcount_inc(&fltr->refcnt);
> +	rcu_read_unlock();
> +	return fltr;
> +}

[ ... ]

> +int bnge_hwrm_set_vnic_filter(struct bnge_net *bn, u16 vnic_id, u16 idx,
> +			      const u8 *mac_addr)
> +{
> +	struct bnge_l2_filter *fltr;
> +	struct bnge_l2_key key;
> +	int rc;
> +
> +	ether_addr_copy(key.dst_mac_addr, mac_addr);
> +	key.vlan = 0;
> +	fltr = bnge_alloc_l2_filter(bn, &key, GFP_KERNEL);
> +	if (IS_ERR(fltr))
> +		return PTR_ERR(fltr);
> +
> +	fltr->base.fw_vnic_id = bn->vnic_info[vnic_id].fw_vnic_id;
> +	rc = bnge_hwrm_l2_filter_alloc(bn->bd, fltr);

[Severity: Medium]
This is a pre-existing issue that this patch only moves. What happens when
a unicast address matches a filter already in bn->l2_fltr_hash_tbl?

For example, "bridge fdb add <own-mac> dev X self" ends up in
dev_uc_add_excl(). At this commit, bnge_cfg_rx_mode() copies every uc
entry and does not skip dev_addr.

In that case bnge_alloc_l2_filter() returns the existing object, with an
extra reference taken in bnge_lookup_l2_filter().
bnge_hwrm_set_vnic_filter() still calls bnge_hwrm_l2_filter_alloc() on
that object, and this overwrites its only firmware handle:

bnge_hwrm_l2_filter_alloc() {
    ...
    fltr->base.filter_id = resp->l2_filter_id;
    ...
}

l2_filters[0] and l2_filters[i] then point to the same object. On
teardown, is the same filter_id freed twice in firmware while the first
firmware filter leaks?

This appears to be fixed later in the series by "bnge: remove refcount
from L2 filter". That commit makes bnge_alloc_l2_filter() return
ERR_PTR(-EEXIST) on a lookup hit. The final bnge_cfg_rx_mode() also
skips entries equal to dev->dev_addr.

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c b/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> new file mode 100644
> index 0000000000000..3a6c8f0a59541
> --- /dev/null
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> @@ -0,0 +1,100 @@

[ ... ]

> +void bnge_set_dflt_rss_indir_tbl(struct bnge_dev *bd)
> +{
> +	u16 max_entries, pad;
> +	u32 *rss_indir_tbl;
> +	u16 i;
> +
> +	max_entries = bnge_get_rxfh_indir_size(bd);
> +	rss_indir_tbl = &bd->rss_indir_tbl[0];
> +
> +	for (i = 0; i < max_entries; i++)
> +		rss_indir_tbl[i] = ethtool_rxfh_indir_default(i,
> +							      bd->rx_nr_rings);

[Severity: Medium]
This isn't a bug introduced by this patch, since the logic is unchanged.
Still, can max_entries be too small for some ring counts?

bnge_get_rxfh_indir_size() returns bnge_cal_nr_rss_ctxs(rx_nr_rings) * 64.
bnge_cal_nr_rss_ctxs() calls bnge_adjust_pow_two(rx_rings - 1, 64):

bnge_resc.h:bnge_adjust_pow_two() {
	u32 blks = total_ent / ent_per_blk;

	if (blks == 0 || blks == 1)
		return ++blks;

	if (!is_power_of_2(blks))
		blks = roundup_pow_of_two(blks);

	return blks;
}

The extra block is added only when blks is 0 or 1. With 129..192 rings,
blks is 2 and comes back unchanged, which gives a 128-entry table. The
same thing happens for 257..320 rings (256 entries) and 513..576 rings
(512 entries).

With 160 RX rings, would queues 128..159 never get default RSS traffic,
both here and in bnge_fill_hw_rss_tbl()? For comparison, the bnxt helper
bnxt_calc_nr_ring_pages() always adds one before rounding up.

bnge_net_init_dflt_rings() sets rx_nr_rings to the minimum of
netif_get_num_default_rss_queues() and the firmware limits, so it does
not have to be a power of two.

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

[ ... ]

> +int bnge_setup_vnic(struct bnge_net *bn, struct bnge_vnic_info *vnic)
> +{
> +	struct bnge_dev *bd = bn->bd;
> +	int rc, i, nr_ctxs;
> +
> +	nr_ctxs = bnge_cal_nr_rss_ctxs(bd->rx_nr_rings);
> +	for (i = 0; i < nr_ctxs; i++) {
> +		rc = bnge_hwrm_vnic_ctx_alloc(bd, vnic, i);

[Severity: Medium]
This is a pre-existing issue, but is nr_ctxs checked against
BNGE_MAX_CTX_PER_VNIC anywhere?

bnge_hwrm_vnic_ctx_alloc() writes vnic->fw_rss_cos_lb_ctx[ctx_idx], and
that array has BNGE_MAX_CTX_PER_VNIC (8) entries. bd->rss_indir_tbl is
always allocated with BNGE_MAX_RSS_TABLE_ENTRIES (512) entries, and
vnic->rss_table is sized by BNGE_MAX_RSS_TABLE_SIZE.

With 577 or more RX rings, bnge_cal_nr_rss_ctxs() returns 16. Then:

- this loop writes past fw_rss_cos_lb_ctx[] for i >= 8

- bnge_set_dflt_rss_indir_tbl() writes 1024 entries into the 512-entry
  rss_indir_tbl, and the u16 pad subtraction wraps

- bnge_fill_hw_rss_tbl() overruns vnic->rss_table

The only limit on rx_nr_rings seems to be in bnge_net_init_dflt_rings().
It comes from netif_get_num_default_rss_queues() and the firmware ring
limits, not from BNGE_MAX_RSS_TABLE_ENTRIES. Triggering this needs a very
large host and matching firmware limits.

Should rx_nr_rings be capped to what these arrays can hold?

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