Re: [net-next, v4 08/10] bnge: add NTUPLE filter support in ethtool

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 8 · Low: 3

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] Once a user ETHER_FLOW rule exists for MAC M, adding M as a
  secondary unicast address (for example a macvlan on top of the port)…
- [Medium] The patch adds CFA_L2_FILTER_ALLOC_REQ_FLAGS_TRAFFIC_L2
  unconditionally in bnge_hwrm_l2_filter_alloc().
- [Medium] This patch makes user filters exist only while NTUPLE is
  enabled (bnge_set_features calls bnge_clear_usr_fltrs when NTUPLE goes…
- [Medium] When a user L2 (ETHER_FLOW) filter is restored on open, it
  reuses a firmware vNIC id that may no longer be valid.
- [Medium] The rule table size reported to userspace is inconsistent and
  too small for the locations the driver hands out.
- [Medium] bnge_add_l2_cls_rule() accepts ETHER_FLOW rules that ask to
  match on fields it does not implement, then drops those fields without…
- [Medium] User filters are kept in software across close, but the close
  path never frees their firmware entries, and the new open path…
- [Medium] ETHER_FLOW ring rules may land on the wrong queue once the RSS
  indirection table has been changed.
- [Medium] If firmware grants fewer RX rings when the interface is
  reopened, restored filters can steer to a different ring than the one…
- [Low] If the caller's rule_locs buffer is too small,
  ETHTOOL_GRXCLSRLALL silently returns a truncated list as a success.
- [Low] bnge_add_l2_cls_rule() requires m_ext->vlan_tci == htons(0xfff)
  but sets key.vlan = ntohs(h_ext->vlan_tci) without masking to…
- [Low] net: bnge: lost destination ring information for RSS-enabled
  NTUPLE filters

--

bnge: add NTUPLE filter support in ethtool

This commit adds get_rxnfc/set_rxnfc ethtool operations to bnge for
listing, inserting and deleting ETHER_FLOW and ntuple rules. User filters
are restored in bnge_open_core() and cleared when NTUPLE is disabled in
bnge_set_features(). It also adds the TRAFFIC_L2 flag to L2 filter
allocation requests.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> index 85dbe64d4c129..8e9cfbad98e04 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c

[ ... ]

> @@ -1169,6 +1170,594 @@ static int bnge_remove_rxfh_context(struct net_device *dev,

[ ... ]

> +static int bnge_grxclsrlall(struct bnge_net *bn, struct ethtool_rxnfc *cmd,
> +			    u32 *rule_locs)
> +{
> +	u32 count;
> +
> +	cmd->data = bn->user_fltr_count;
> +	rcu_read_lock();
> +	count = bnge_get_all_fltr_ids_rcu(bn, bn->l2_fltr_hash_tbl,
> +					  BNGE_L2_FLTR_HASH_SIZE, rule_locs, 0,
> +					  cmd->rule_cnt, 0);
> +	cmd->rule_cnt = bnge_get_all_fltr_ids_rcu(bn, bn->ntp_fltr_hash_tbl,
> +						  BNGE_NTP_FLTR_HASH_SIZE,
> +						  rule_locs, count,
> +						  cmd->rule_cnt,
> +						  BNGE_MAX_L2_FLTRS);
> +	rcu_read_unlock();
> +
> +	return 0;
> +}

[Severity: Low]
What happens when rule_cnt is smaller than the number of installed rules?
bnge_get_all_fltr_ids_rcu() stops once id_cnt entries have been copied and
doesn't check whether more remain. bnge_grxclsrlall() then always returns 0.

The get_rxnfc documentation in include/linux/ethtool.h says drivers must
return -EMSGSIZE when rule_cnt is too small. With two rules installed and
rule_cnt = 1, userspace would get one rule and no error. The same thing can
happen if rules are added between ETHTOOL_GRXCLSRLCNT and
ETHTOOL_GRXCLSRLALL.

[ ... ]

> +static int bnge_add_l2_cls_rule(struct bnge_net *bn,
> +				struct ethtool_rx_flow_spec *fs)
> +{

[ ... ]

> +	if (!is_broadcast_ether_addr(m_ether->h_dest))
> +		return -EINVAL;
> +
> +	if (is_broadcast_ether_addr(h_ether->h_dest) ||
> +	    is_multicast_ether_addr(h_ether->h_dest))
> +		return -EINVAL;
> +
> +	ether_addr_copy(key.dst_mac_addr, h_ether->h_dest);
> +	key.vlan = 0;

[Severity: Medium]
Should this also reject masks for fields the filter doesn't implement?
Only the dst MAC mask and the VLAN TCI mask are checked, and only dst MAC
and VLAN are copied into key.

Non-zero masks in m_u.ether_spec.h_source and m_u.ether_spec.h_proto are
silently dropped. The same goes for m_ext.vlan_etype and m_ext.data when
FLOW_EXT is set. ethtool_set_rxnfc() in net/ethtool/ioctl.c doesn't check
these masks either.

For example, 'flow-type ether dst M src S proto 0x0800 action 3' would be
installed as a dst-only rule. It would then steer traffic that doesn't
match the requested source or EtherType.

> +	if (fs->flow_type & FLOW_EXT) {
> +		struct ethtool_flow_ext *m_ext = &fs->m_ext;
> +		struct ethtool_flow_ext *h_ext = &fs->h_ext;
> +
> +		if (m_ext->vlan_tci != htons(0xfff) || !h_ext->vlan_tci)
> +			return -EINVAL;
> +		key.vlan = ntohs(h_ext->vlan_tci);
> +	}

[Severity: Low]
The mask has to be htons(0xfff), but key.vlan isn't masked with
VLAN_VID_MASK. Can PCP/DEI bits end up in the software key and in
req->l2_ivlan, while l2_ivlan_mask is 0xfff?

bnge_lookup_l2_filter() compares the full vlan. So two rules with the same
VID but different PCP would pass the duplicate check and create duplicate
hardware filters.

A priority-tagged TCI such as 0x2000 would also pass the !h_ext->vlan_tci
check and install a num_vlans = 1 filter on VID 0.

> +
> +	flags = BNGE_ACT_RING_DST;
> +	vnic_id = bn->vnic_info[BNGE_VNIC_DEFAULT].fw_vnic_id;

[Severity: Medium]
Do ETHER_FLOW ring rules still reach the requested ring after the RSS
indirection table has been changed?

With BNGE_ACT_RING_DST set, bnge_hwrm_l2_filter_alloc() programs dst_id as
the default vNIC and uses rxq as the ring table index:

	if (BNGE_L2_FLTR_IS_NTP_MAC(fltr)) {
		req->enables |= cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_ENABLES_RFS_RING_TBL_IDX);
		req->rfs_ring_tbl_idx = cpu_to_le16(fltr->base.rxq);
	}

The ntuple path in bnge_cfg_rfs_ring_tbl_idx() uses BNGE_VNIC_NTUPLE
instead. bnge_fill_hw_rss_tbl() gives only that vNIC an identity table, via
ethtool_rxfh_indir_default(). The default vNIC's table comes from
bd->rss_indir_tbl.

After 'ethtool -X', wouldn't matching packets go to rss_indir_tbl[ring]
while bnge_grxclsrule() still reports the requested ring?

For comparison, bnxt doesn't use RFS_RING_TBL_IDX on L2 filters and refuses
ETHER_FLOW ring rules on P5+ chips.

> +
> +	fltr = bnge_alloc_user_l2_filter(bn, &key, flags);
> +	if (IS_ERR(fltr))
> +		return PTR_ERR(fltr);

[Severity: High]
Can a user ETHER_FLOW rule for MAC M break RX filtering once M is also
added as a secondary unicast address, for example by a macvlan on top of
the port?

User L2 rules now go into the same l2_fltr_hash_tbl as the vNIC unicast
filters. bnge_alloc_l2_filter() returns ERR_PTR(-EEXIST) when the
{mac, vlan} key is already there. bnge_cfg_rx_mode() only skips uc entries
equal to dev_addr, so M reaches bnge_hwrm_set_vnic_filter():

bnge_set_rx_mode()
  bnge_cfg_rx_mode()
    bnge_hwrm_set_vnic_filter()
      bnge_alloc_l2_filter()      -> -EEXIST
    vnic->uc_filter_count = i;
    return rc;

This returns before the skip_uc label, so bnge_hwrm_cfa_l2_set_rx_mask()
never runs and the bcast/mcast/promisc mask updates are lost.

User L2 filters also survive close. bnge_hwrm_clear_vnic_filter() only
frees vnic->l2_filters[], and bnge_free_l2_filters() is only called from
bnge_netdev_free(). On the next open:

bnge_init_chip()
  bnge_cfg_rx_mode()              -> -EEXIST
  else if (rc)
      goto err_out;

Wouldn't the interface then fail to open until the ethtool rule is deleted?
In that case bnge_cfg_usr_fltrs() is never reached either.

Before this patch only vNIC unicast filters could be in the L2 hash, so
this collision couldn't happen.

[ ... ]

> +static int bnge_srxclsrldel(struct bnge_net *bn, struct ethtool_rxnfc *cmd)
> +{

[ ... ]

> +	if (fltr_base) {
> +		struct bnge_l2_filter *l2_fltr;
> +
> +		l2_fltr = container_of(fltr_base, struct bnge_l2_filter, base);
> +		rcu_read_unlock();
> +		bnge_hwrm_l2_filter_free(bn->bd, l2_fltr);
> +		bnge_del_l2_filter_rcu(bn, l2_fltr);
> +		return 0;
> +	}

[Severity: Medium]
Are the firmware entries for user filters ever freed across close/open?
User filters stay in software over close, but the close path doesn't free
them in firmware:

- bnge_hwrm_clear_vnic_filter() only frees vnic->l2_filters[]
- bnge_free_core() -> bnge_free_ntp_fltrs(bn, true) skips user filters and
  issues no HWRM call
- on open, bnge_cfg_usr_fltrs() allocates every user filter in firmware
  again and overwrites base.filter_id

If firmware doesn't free these implicitly, each close/open cycle leaks the
previous entries. bnge_clear_usr_fltrs() has the same problem when NTUPLE
is turned off. That can only happen while the interface is down, because
fix_features blocks the change while it is running.

If firmware does free them with the vNIC, then deleting an L2 rule here
while the interface is down sends HWRM_CFA_L2_FILTER_FREE with the
filter_id from the previous up cycle. Unlike the ntuple free helper,
bnge_hwrm_l2_filter_free() has no BNGE_STATE_OPEN check.

Could that command fail, or free an unrelated filter if the id has been
reused?

[ ... ]

> +	switch (cmd->cmd) {
> +	case ETHTOOL_GRXCLSRLCNT:
> +		cmd->rule_cnt = bn->user_fltr_count;
> +		cmd->data = bd->max_fltr | RX_CLS_LOC_SPECIAL;
> +		break;

[Severity: Medium]
Is this the right rule table size? bd->max_fltr is BNGE_MAX_NTUPLE_FLTRS
and doesn't include the L2 range. Ntuple rules, however, are returned at:

	fs->location = BNGE_MAX_L2_FLTRS + new_fltr->base.sw_id;

bnge_grxclsrule() also accepts locations up to
BNGE_MAX_L2_FLTRS + bd->max_fltr. So valid locations can go past the
advertised size.

bnge_grxclsrlall() reports a rule count instead of the table size:

	cmd->data = bn->user_fltr_count;

So a single ntuple rule at location 128 gives @data = 1.

include/uapi/linux/ethtool.h defines @data as the size of the rule table
for both ETHTOOL_GRXCLSRLCNT and ETHTOOL_GRXCLSRLALL. bnxt avoids the first
mismatch because its max_fltr includes BNXT_L2_FLTR_MAX_FLTR.

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> index 7fbb67021b5d9..764c967c64572 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> @@ -577,3 +577,63 @@ int bnge_hwrm_set_vnic_filter(struct bnge_net *bn, u16 vnic_id, u16 idx,

[ ... ]

> +	if (fltr->type == BNGE_FLTR_TYPE_NTUPLE) {
> +		ntp_fltr = container_of(fltr, struct bnge_ntuple_filter, base);
> +		l2_fltr = bn->vnic_info[BNGE_VNIC_DEFAULT].l2_filters[0];
> +		ntp_fltr->l2_filter_id = l2_fltr->base.filter_id;
> +		if (bnge_hwrm_cfa_ntuple_filter_alloc(bn->bd, ntp_fltr)) {

[Severity: Medium]
Can the saved base.rxq be out of range here? On open,
bnge_reserve_rings() can lower bd->rx_nr_rings and log "RX rings resv
reduced". It handles the RSS indirection table through
ethtool_rxfh_indir_lost(), but it doesn't touch ring-steering filters.

This path reprograms each filter with its old rxq and doesn't check it
against the new rx_nr_rings. The NTUPLE vNIC's ring table is
ethtool_rxfh_indir_default(i, rx_nr_rings).

Wouldn't an rxq at or above the new count then resolve to
rxq % rx_nr_rings, while get_rxnfc still reports the old ring?

> +			netdev_err(bn->netdev,
> +				   "restoring previously configured ntuple filter id %d failed\n",
> +				   fltr->sw_id);
> +			bnge_del_ntp_filter_rcu(bn, ntp_fltr);
> +		}
> +	} else if (fltr->type == BNGE_FLTR_TYPE_L2) {
> +		l2_fltr = container_of(fltr, struct bnge_l2_filter, base);
> +		if (bnge_hwrm_l2_filter_alloc(bn->bd, l2_fltr)) {

[Severity: Medium]
Is fltr->base.fw_vnic_id still valid at this point? bnge_add_l2_cls_rule()
saved the default vNIC's firmware id when the rule was added:

	vnic_id = bn->vnic_info[BNGE_VNIC_DEFAULT].fw_vnic_id;
	...
	fltr->base.fw_vnic_id = vnic_id;

bnge_clear_vnic() frees that vNIC on close, and on open
bnge_hwrm_vnic_alloc() gets a new id from firmware (resp->vnic_id). This
branch doesn't refresh fw_vnic_id, so req->dst_id still carries the old id.

The ntuple branch above does refresh its per-open handle (l2_filter_id).

If firmware hands out a different vNIC id, wouldn't the restored rule point
at a stale vNIC? Or, if firmware rejects the id, the rule would be removed
by bnge_del_l2_filter_rcu() with only a log message.

[ ... ]

> +void bnge_cfg_usr_fltrs(struct bnge_net *bn)
> +{
> +	struct bnge_filter_base *usr_fltr, *tmp;
> +
> +	list_for_each_entry_safe(usr_fltr, tmp, &bn->usr_fltr_list, list_node)
> +		bnge_cfg_one_usr_fltr(bn, usr_fltr);
> +}

[Severity: Medium]
Should this check BNGE_NET_EN_NTUPLE before reprogramming filters?

User filters are cleared when NTUPLE is turned off through
bnge_set_features(). However, bnge_init_chip() can also turn it off when
bnge_alloc_rfs_vnic() fails during open, and it doesn't clear user filters:

	bd->flags &= ~BNGE_EN_ARFS_CAP;
	bn->priv_flags &= ~BNGE_NET_EN_NTUPLE;
	bn->netdev->features &= ~NETIF_F_NTUPLE;

bnge_open_core() then calls bnge_cfg_usr_fltrs(), which restores every
saved filter. DROP and L2 filters stay active. Non-RSS ntuple filters get
dst_id = vnic_info[BNGE_VNIC_NTUPLE].fw_vnic_id from
bnge_cfg_rfs_ring_tbl_idx(), and that is the vNIC that just failed.

A later 'ethtool -K ntuple off' would also do nothing. bnge_set_features()
returns early when flags == priv_flags, so bnge_clear_usr_fltrs() never
runs.

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> index 3eb67379c1a57..473d1aec80f38 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> @@ -906,15 +906,17 @@ int bnge_hwrm_l2_filter_alloc(struct bnge_dev *bd, struct bnge_l2_filter *fltr)

[ ... ]

> -	req->flags = cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_FLAGS_PATH_RX);
> -
> -	req->flags |= cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_FLAGS_OUTERMOST);
> +	flags = CFA_L2_FILTER_ALLOC_REQ_FLAGS_PATH_RX |
> +		CFA_L2_FILTER_ALLOC_REQ_FLAGS_TRAFFIC_L2 |
> +		CFA_L2_FILTER_ALLOC_REQ_FLAGS_OUTERMOST;
> +	req->flags = cpu_to_le32(flags);

[Severity: Medium]
CFA_L2_FILTER_ALLOC_REQ_FLAGS_TRAFFIC_L2 is now set for every L2 filter.
That includes the default vNIC MAC filters that bnge_hwrm_set_vnic_filter()
installs on every open.

The traffic field used to be 0 (TRAFFIC_NO_ROCE_L2), and bnxt doesn't set
this flag. Is this change intended, and could it affect RoCE/L2 steering
for the aux RoCE driver?

The commit message doesn't mention this change. It also doesn't mention
that user filters are now restored on open (bnge_cfg_usr_fltrs()) and
flushed when NTUPLE is disabled (bnge_clear_usr_fltrs()).

The commit message says:

  Since with ethtool support user filters creation is supported, when
  interface is down clearing of filters which are in software remains
  intact.

Isn't this already the existing behaviour? bnge_free_core() already calls
bnge_free_ntp_fltrs(bn, true) under the comment "Free non-user filters
only", and this patch doesn't change that.

[ ... ]

> @@ -1169,6 +1170,594 @@ static int bnge_remove_rxfh_context(struct net_device *dev,
>  	return 0;
>  }
>  
> +#define BNGE_IP_PROTO_FULL_MASK	0xFF
> +#define BNGE_IP_PROTO_WILDCARD	0x0
> +
> +static u32 bnge_get_all_fltr_ids_rcu(struct bnge_net *bn,
> +				     struct hlist_head tbl[],
> +				     u32 tbl_size, u32 *ids, u32 start,
> +				     u32 id_cnt, u32 offset)
> +{
> +	u32 i, j = start;
> +
> +	if (j >= id_cnt)
> +		return j;
> +
> +	for (i = 0; i < tbl_size; i++) {
> +		struct bnge_filter_base *fltr;
> +		struct hlist_head *head;
> +
> +		head = &tbl[i];
> +		hlist_for_each_entry_rcu(fltr, head, hlist) {
> +			if (!fltr->flags ||
> +			    test_bit(BNGE_FLTR_FW_DELETED, &fltr->state))
> +				continue;
> +			ids[j++] = fltr->sw_id + offset;
> +			if (j == id_cnt)
> +				return j;
> +		}
> +	}
> +	return j;
> +}
> +
> +static struct bnge_filter_base *bnge_get_one_fltr_rcu(struct bnge_net *bn,
> +						      struct hlist_head tbl[],
> +						      u32 tbl_size, u32 id,
> +						      u32 offset)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < tbl_size; i++) {
> +		struct bnge_filter_base *fltr;
> +		struct hlist_head *head;
> +
> +		head = &tbl[i];
> +		hlist_for_each_entry_rcu(fltr, head, hlist) {
> +			if (fltr->flags && fltr->sw_id + offset == id)
> +				return fltr;
> +		}
> +	}
> +	return NULL;
> +}
> +
> +static int bnge_grxclsrlall(struct bnge_net *bn, struct ethtool_rxnfc *cmd,
> +			    u32 *rule_locs)
> +{
> +	u32 count;
> +
> +	cmd->data = bn->user_fltr_count;
> +	rcu_read_lock();
> +	count = bnge_get_all_fltr_ids_rcu(bn, bn->l2_fltr_hash_tbl,
> +					  BNGE_L2_FLTR_HASH_SIZE, rule_locs, 0,
> +					  cmd->rule_cnt, 0);
> +	cmd->rule_cnt = bnge_get_all_fltr_ids_rcu(bn, bn->ntp_fltr_hash_tbl,
> +						  BNGE_NTP_FLTR_HASH_SIZE,
> +						  rule_locs, count,
> +						  cmd->rule_cnt,
> +						  BNGE_MAX_L2_FLTRS);
> +	rcu_read_unlock();
> +
> +	return 0;
> +}
> +
> +static int bnge_grxclsrule(struct bnge_net *bn, struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fs =
> +		(struct ethtool_rx_flow_spec *)&cmd->fs;
> +	struct bnge_filter_base *fltr_base;
> +	struct bnge_ntuple_filter *fltr;
> +	struct bnge_flow_masks *fmasks;
> +	struct bnge_dev *bd = bn->bd;
> +	struct flow_keys *fkeys;
> +	int rc = -EINVAL;
> +
> +	if (fs->location >= BNGE_MAX_L2_FLTRS + bd->max_fltr)
> +		return rc;
> +
> +	rcu_read_lock();
> +	fltr_base = bnge_get_one_fltr_rcu(bn, bn->l2_fltr_hash_tbl,
> +					  BNGE_L2_FLTR_HASH_SIZE,
> +					  fs->location, 0);
> +	if (fltr_base) {
> +		struct ethhdr *h_ether = &fs->h_u.ether_spec;
> +		struct ethhdr *m_ether = &fs->m_u.ether_spec;
> +		struct bnge_l2_filter *l2_fltr;
> +		struct bnge_l2_key *l2_key;
> +
> +		l2_fltr = container_of(fltr_base, struct bnge_l2_filter, base);
> +		l2_key = &l2_fltr->l2_key;
> +		fs->flow_type = ETHER_FLOW;
> +		ether_addr_copy(h_ether->h_dest, l2_key->dst_mac_addr);
> +		eth_broadcast_addr(m_ether->h_dest);
> +		if (l2_key->vlan) {
> +			struct ethtool_flow_ext *m_ext = &fs->m_ext;
> +			struct ethtool_flow_ext *h_ext = &fs->h_ext;
> +
> +			fs->flow_type |= FLOW_EXT;
> +			m_ext->vlan_tci = htons(0xfff);
> +			h_ext->vlan_tci = htons(l2_key->vlan);
> +		}
> +		if (fltr_base->flags & BNGE_ACT_RING_DST)
> +			fs->ring_cookie = fltr_base->rxq;
> +		rcu_read_unlock();
> +		return 0;
> +	}
> +	fltr_base = bnge_get_one_fltr_rcu(bn, bn->ntp_fltr_hash_tbl,
> +					  BNGE_NTP_FLTR_HASH_SIZE,
> +					  fs->location, BNGE_MAX_L2_FLTRS);
> +	if (!fltr_base) {
> +		rcu_read_unlock();
> +		return rc;
> +	}
> +	fltr = container_of(fltr_base, struct bnge_ntuple_filter, base);
> +
> +	fkeys = &fltr->fkeys;
> +	fmasks = &fltr->fmasks;
> +	if (fkeys->basic.n_proto == htons(ETH_P_IP)) {
> +		if (fkeys->basic.ip_proto == BNGE_IP_PROTO_WILDCARD) {
> +			fs->flow_type = IP_USER_FLOW;
> +			fs->h_u.usr_ip4_spec.ip_ver = ETH_RX_NFC_IP4;
> +			fs->h_u.usr_ip4_spec.proto = BNGE_IP_PROTO_WILDCARD;
> +			fs->m_u.usr_ip4_spec.proto = 0;
> +		} else if (fkeys->basic.ip_proto == IPPROTO_ICMP) {
> +			fs->flow_type = IP_USER_FLOW;
> +			fs->h_u.usr_ip4_spec.ip_ver = ETH_RX_NFC_IP4;
> +			fs->h_u.usr_ip4_spec.proto = IPPROTO_ICMP;
> +			fs->m_u.usr_ip4_spec.proto = BNGE_IP_PROTO_FULL_MASK;
> +		} else if (fkeys->basic.ip_proto == IPPROTO_TCP) {
> +			fs->flow_type = TCP_V4_FLOW;
> +		} else if (fkeys->basic.ip_proto == IPPROTO_UDP) {
> +			fs->flow_type = UDP_V4_FLOW;
> +		} else {
> +			goto fltr_err;
> +		}
> +
> +		fs->h_u.tcp_ip4_spec.ip4src = fkeys->addrs.v4addrs.src;
> +		fs->m_u.tcp_ip4_spec.ip4src = fmasks->addrs.v4addrs.src;
> +		fs->h_u.tcp_ip4_spec.ip4dst = fkeys->addrs.v4addrs.dst;
> +		fs->m_u.tcp_ip4_spec.ip4dst = fmasks->addrs.v4addrs.dst;
> +		if (fs->flow_type == TCP_V4_FLOW ||
> +		    fs->flow_type == UDP_V4_FLOW) {
> +			fs->h_u.tcp_ip4_spec.psrc = fkeys->ports.src;
> +			fs->m_u.tcp_ip4_spec.psrc = fmasks->ports.src;
> +			fs->h_u.tcp_ip4_spec.pdst = fkeys->ports.dst;
> +			fs->m_u.tcp_ip4_spec.pdst = fmasks->ports.dst;
> +		}
> +	} else {
> +		if (fkeys->basic.ip_proto == BNGE_IP_PROTO_WILDCARD) {
> +			fs->flow_type = IPV6_USER_FLOW;
> +			fs->h_u.usr_ip6_spec.l4_proto =
> +				BNGE_IP_PROTO_WILDCARD;
> +			fs->m_u.usr_ip6_spec.l4_proto = 0;
> +		} else if (fkeys->basic.ip_proto == IPPROTO_ICMPV6) {
> +			fs->flow_type = IPV6_USER_FLOW;
> +			fs->h_u.usr_ip6_spec.l4_proto = IPPROTO_ICMPV6;
> +			fs->m_u.usr_ip6_spec.l4_proto =
> +				BNGE_IP_PROTO_FULL_MASK;
> +		} else if (fkeys->basic.ip_proto == IPPROTO_TCP) {
> +			fs->flow_type = TCP_V6_FLOW;
> +		} else if (fkeys->basic.ip_proto == IPPROTO_UDP) {
> +			fs->flow_type = UDP_V6_FLOW;
> +		} else {
> +			goto fltr_err;
> +		}
> +
> +		memcpy(fs->h_u.tcp_ip6_spec.ip6src,
> +		       &fkeys->addrs.v6addrs.src,
> +		       sizeof(struct in6_addr));
> +		memcpy(fs->m_u.tcp_ip6_spec.ip6src,
> +		       &fmasks->addrs.v6addrs.src,
> +		       sizeof(struct in6_addr));
> +		memcpy(fs->h_u.tcp_ip6_spec.ip6dst,
> +		       &fkeys->addrs.v6addrs.dst,
> +		       sizeof(struct in6_addr));
> +		memcpy(fs->m_u.tcp_ip6_spec.ip6dst,
> +		       &fmasks->addrs.v6addrs.dst,
> +		       sizeof(struct in6_addr));
> +
> +		if (fs->flow_type == TCP_V6_FLOW ||
> +		    fs->flow_type == UDP_V6_FLOW) {
> +			fs->h_u.tcp_ip6_spec.psrc = fkeys->ports.src;
> +			fs->m_u.tcp_ip6_spec.psrc = fmasks->ports.src;
> +			fs->h_u.tcp_ip6_spec.pdst = fkeys->ports.dst;
> +			fs->m_u.tcp_ip6_spec.pdst = fmasks->ports.dst;
> +		}
> +	}
> +
> +	if (fltr->base.flags & BNGE_ACT_DROP) {
> +		fs->ring_cookie = RX_CLS_FLOW_DISC;
> +	} else if (fltr->base.flags & BNGE_ACT_RSS_CTX) {
> +		fs->flow_type |= FLOW_RSS;
> +		cmd->rss_context = fltr->base.fw_vnic_id;
> +	} else {
> +		fs->ring_cookie = fltr->base.rxq;
> +	}
> +	rc = 0;
> +
> +fltr_err:
> +	rcu_read_unlock();
> +
> +	return rc;
> +}
> +
> +static bool bnge_verify_ntuple_ip4_flow(struct ethtool_usrip4_spec *ip_spec,
> +					struct ethtool_usrip4_spec *ip_mask)
> +{
> +	u8 mproto = ip_mask->proto;
> +	u8 sproto = ip_spec->proto;
> +
> +	if (ip_mask->l4_4_bytes || ip_mask->tos ||
> +	    ip_spec->ip_ver != ETH_RX_NFC_IP4 ||
> +	    (mproto && (mproto != BNGE_IP_PROTO_FULL_MASK ||
> +			sproto != IPPROTO_ICMP)))
> +		return false;
> +	return true;
> +}
> +
> +static bool bnge_verify_ntuple_ip6_flow(struct ethtool_usrip6_spec *ip_spec,
> +					struct ethtool_usrip6_spec *ip_mask)
> +{
> +	u8 mproto = ip_mask->l4_proto;
> +	u8 sproto = ip_spec->l4_proto;
> +
> +	if (ip_mask->l4_4_bytes || ip_mask->tclass ||
> +	    (mproto && (mproto != BNGE_IP_PROTO_FULL_MASK ||
> +			sproto != IPPROTO_ICMPV6)))
> +		return false;
> +	return true;
> +}
> +
> +static int bnge_add_ntuple_cls_rule(struct bnge_net *bn,
> +				    struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fs = &cmd->fs;
> +	struct bnge_ntuple_filter *new_fltr, *fltr;
> +	u32 flow_type = fs->flow_type & 0xff;
> +	struct bnge_l2_filter *l2_fltr;
> +	struct bnge_flow_masks *fmasks;
> +	struct flow_keys *fkeys;
> +	u32 idx;
> +	int rc;
> +
> +	if (!bn->vnic_info)
> +		return -EAGAIN;
> +
> +	if (fs->flow_type & (FLOW_MAC_EXT | FLOW_EXT))
> +		return -EOPNOTSUPP;
> +
> +	if (fs->ring_cookie != RX_CLS_FLOW_DISC &&
> +	    ethtool_get_flow_spec_ring_vf(fs->ring_cookie))
> +		return -EOPNOTSUPP;
> +
> +	if (flow_type == IP_USER_FLOW) {
> +		if (!bnge_verify_ntuple_ip4_flow(&fs->h_u.usr_ip4_spec,
> +						 &fs->m_u.usr_ip4_spec))
> +			return -EOPNOTSUPP;
> +	}
> +
> +	if (flow_type == IPV6_USER_FLOW) {
> +		if (!bnge_verify_ntuple_ip6_flow(&fs->h_u.usr_ip6_spec,
> +						 &fs->m_u.usr_ip6_spec))
> +			return -EOPNOTSUPP;
> +	}
> +
> +	new_fltr = kzalloc_obj(*new_fltr, GFP_KERNEL);
> +	if (!new_fltr)
> +		return -ENOMEM;
> +
> +	l2_fltr = bn->vnic_info[BNGE_VNIC_DEFAULT].l2_filters[0];
> +	new_fltr->l2_filter_id = l2_fltr->base.filter_id;
> +	fmasks = &new_fltr->fmasks;
> +	fkeys = &new_fltr->fkeys;
> +
> +	rc = -EOPNOTSUPP;
> +	switch (flow_type) {
> +	case IP_USER_FLOW: {
> +		struct ethtool_usrip4_spec *ip_spec = &fs->h_u.usr_ip4_spec;
> +		struct ethtool_usrip4_spec *ip_mask = &fs->m_u.usr_ip4_spec;
> +
> +		fkeys->basic.ip_proto = ip_mask->proto ? ip_spec->proto
> +						       : BNGE_IP_PROTO_WILDCARD;
> +		fkeys->basic.n_proto = htons(ETH_P_IP);
> +		fkeys->addrs.v4addrs.src = ip_spec->ip4src;
> +		fmasks->addrs.v4addrs.src = ip_mask->ip4src;
> +		fkeys->addrs.v4addrs.dst = ip_spec->ip4dst;
> +		fmasks->addrs.v4addrs.dst = ip_mask->ip4dst;
> +		break;
> +	}
> +	case TCP_V4_FLOW:
> +	case UDP_V4_FLOW: {
> +		struct ethtool_tcpip4_spec *ip_spec = &fs->h_u.tcp_ip4_spec;
> +		struct ethtool_tcpip4_spec *ip_mask = &fs->m_u.tcp_ip4_spec;
> +
> +		if (ip_mask->tos)
> +			goto err_free_fltr;
> +
> +		fkeys->basic.ip_proto = IPPROTO_TCP;
> +		if (flow_type == UDP_V4_FLOW)
> +			fkeys->basic.ip_proto = IPPROTO_UDP;
> +		fkeys->basic.n_proto = htons(ETH_P_IP);
> +		fkeys->addrs.v4addrs.src = ip_spec->ip4src;
> +		fmasks->addrs.v4addrs.src = ip_mask->ip4src;
> +		fkeys->addrs.v4addrs.dst = ip_spec->ip4dst;
> +		fmasks->addrs.v4addrs.dst = ip_mask->ip4dst;
> +		fkeys->ports.src = ip_spec->psrc;
> +		fmasks->ports.src = ip_mask->psrc;
> +		fkeys->ports.dst = ip_spec->pdst;
> +		fmasks->ports.dst = ip_mask->pdst;
> +		break;
> +	}
> +	case IPV6_USER_FLOW: {
> +		struct ethtool_usrip6_spec *ip_spec = &fs->h_u.usr_ip6_spec;
> +		struct ethtool_usrip6_spec *ip_mask = &fs->m_u.usr_ip6_spec;
> +
> +		fkeys->basic.ip_proto = ip_mask->l4_proto ? ip_spec->l4_proto
> +					: BNGE_IP_PROTO_WILDCARD;
> +		fkeys->basic.n_proto = htons(ETH_P_IPV6);
> +
> +		memcpy(&fkeys->addrs.v6addrs.src, ip_spec->ip6src,
> +		       sizeof(struct in6_addr));
> +		memcpy(&fmasks->addrs.v6addrs.src, ip_mask->ip6src,
> +		       sizeof(struct in6_addr));
> +		memcpy(&fkeys->addrs.v6addrs.dst, ip_spec->ip6dst,
> +		       sizeof(struct in6_addr));
> +		memcpy(&fmasks->addrs.v6addrs.dst, ip_mask->ip6dst,
> +		       sizeof(struct in6_addr));
> +		break;
> +	}
> +	case TCP_V6_FLOW:
> +	case UDP_V6_FLOW: {
> +		struct ethtool_tcpip6_spec *ip_spec = &fs->h_u.tcp_ip6_spec;
> +		struct ethtool_tcpip6_spec *ip_mask = &fs->m_u.tcp_ip6_spec;
> +
> +		if (ip_mask->tclass)
> +			goto err_free_fltr;
> +
> +		fkeys->basic.ip_proto = IPPROTO_TCP;
> +		if (flow_type == UDP_V6_FLOW)
> +			fkeys->basic.ip_proto = IPPROTO_UDP;
> +		fkeys->basic.n_proto = htons(ETH_P_IPV6);
> +
> +		memcpy(&fkeys->addrs.v6addrs.src, ip_spec->ip6src,
> +		       sizeof(struct in6_addr));
> +		memcpy(&fmasks->addrs.v6addrs.src, ip_mask->ip6src,
> +		       sizeof(struct in6_addr));
> +		memcpy(&fkeys->addrs.v6addrs.dst, ip_spec->ip6dst,
> +		       sizeof(struct in6_addr));
> +		memcpy(&fmasks->addrs.v6addrs.dst, ip_mask->ip6dst,
> +		       sizeof(struct in6_addr));
> +
> +		fkeys->ports.src = ip_spec->psrc;
> +		fmasks->ports.src = ip_mask->psrc;
> +		fkeys->ports.dst = ip_spec->pdst;
> +		fmasks->ports.dst = ip_mask->pdst;
> +		break;
> +	}
> +	default:
> +		rc = -EOPNOTSUPP;
> +		goto err_free_fltr;
> +	}
> +	if (!memcmp(&BNGE_FLOW_MASK_NONE, fmasks, sizeof(*fmasks)))
> +		goto err_free_fltr;
> +
> +	idx = bnge_get_ntp_filter_idx(bn, fkeys, NULL);
> +	rcu_read_lock();
> +	fltr = bnge_lookup_ntp_filter_from_idx(bn, new_fltr, idx);
> +	if (fltr) {
> +		rcu_read_unlock();
> +		rc = -EEXIST;
> +		goto err_free_fltr;
> +	}
> +	rcu_read_unlock();
> +
> +	new_fltr->base.flags = BNGE_ACT_NO_AGING;
> +	if (fs->flow_type & FLOW_RSS) {
> +		struct bnge_rss_ctx *rss_ctx;
> +
> +		new_fltr->base.fw_vnic_id = 0;
> +		new_fltr->base.flags |= BNGE_ACT_RSS_CTX;
> +		rss_ctx = bnge_get_rss_ctx_from_index(bn, cmd->rss_context);
> +		if (rss_ctx) {
> +			new_fltr->base.fw_vnic_id = rss_ctx->index;
> +		} else {
> +			rc = -EINVAL;
> +			goto err_free_fltr;
> +		}
> +	}
> +	if (fs->ring_cookie == RX_CLS_FLOW_DISC)
> +		new_fltr->base.flags |= BNGE_ACT_DROP;
> +	else
> +		new_fltr->base.rxq = ethtool_get_flow_spec_ring(fs->ring_cookie);
> +	__set_bit(BNGE_FLTR_VALID, &new_fltr->base.state);
> +	rc = bnge_insert_ntp_filter(bn, new_fltr, idx);
> +	if (!rc) {
> +		rc = bnge_hwrm_cfa_ntuple_filter_alloc(bn->bd, new_fltr);
> +		if (rc) {
> +			bnge_del_ntp_filter_rcu(bn, new_fltr);
> +			return rc;
> +		}
> +		fs->location = BNGE_MAX_L2_FLTRS + new_fltr->base.sw_id;
> +		return 0;
> +	}
> +
> +err_free_fltr:
> +	kfree(new_fltr);
> +	return rc;
> +}
> +
> +static int bnge_add_l2_cls_rule(struct bnge_net *bn,
> +				struct ethtool_rx_flow_spec *fs)
> +{
> +	u32 ring = ethtool_get_flow_spec_ring(fs->ring_cookie);
> +	struct ethhdr *h_ether = &fs->h_u.ether_spec;
> +	struct ethhdr *m_ether = &fs->m_u.ether_spec;
> +	struct bnge_l2_filter *fltr;
> +	struct bnge_l2_key key;
> +	u16 vnic_id;
> +	u8 flags;
> +	int rc;
> +
> +	if (ethtool_get_flow_spec_ring_vf(fs->ring_cookie))
> +		return -EOPNOTSUPP;
> +
> +	if (!is_broadcast_ether_addr(m_ether->h_dest))
> +		return -EINVAL;
> +
> +	if (is_broadcast_ether_addr(h_ether->h_dest) ||
> +	    is_multicast_ether_addr(h_ether->h_dest))
> +		return -EINVAL;
> +
> +	ether_addr_copy(key.dst_mac_addr, h_ether->h_dest);
> +	key.vlan = 0;
> +	if (fs->flow_type & FLOW_EXT) {
> +		struct ethtool_flow_ext *m_ext = &fs->m_ext;
> +		struct ethtool_flow_ext *h_ext = &fs->h_ext;
> +
> +		if (m_ext->vlan_tci != htons(0xfff) || !h_ext->vlan_tci)
> +			return -EINVAL;
> +		key.vlan = ntohs(h_ext->vlan_tci);
> +	}
> +
> +	flags = BNGE_ACT_RING_DST;
> +	vnic_id = bn->vnic_info[BNGE_VNIC_DEFAULT].fw_vnic_id;
> +
> +	fltr = bnge_alloc_user_l2_filter(bn, &key, flags);
> +	if (IS_ERR(fltr))
> +		return PTR_ERR(fltr);
> +
> +	fltr->base.fw_vnic_id = vnic_id;
> +	fltr->base.rxq = ring;
> +	rc = bnge_hwrm_l2_filter_alloc(bn->bd, fltr);
> +	if (rc)
> +		bnge_del_l2_filter_rcu(bn, fltr);
> +	else
> +		fs->location = fltr->base.sw_id;
> +	return rc;
> +}
> +
> +static int bnge_srxclsrlins(struct bnge_net *bn, struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fs = &cmd->fs;
> +	struct bnge_dev *bd = bn->bd;
> +	u32 ring, flow_type;
> +	int rc;
> +
> +	if (!netif_running(bn->netdev))
> +		return -EAGAIN;
> +	if (!(bn->priv_flags & BNGE_NET_EN_NTUPLE))
> +		return -EPERM;
> +	if (fs->location != RX_CLS_LOC_ANY)
> +		return -EINVAL;
> +
> +	flow_type = fs->flow_type;
> +
> +	if (flow_type & FLOW_MAC_EXT)
> +		return -EINVAL;
> +
> +	flow_type &= ~FLOW_EXT;
> +
> +	if (fs->ring_cookie == RX_CLS_FLOW_DISC && flow_type != ETHER_FLOW)
> +		return bnge_add_ntuple_cls_rule(bn, cmd);
> +
> +	ring = ethtool_get_flow_spec_ring(fs->ring_cookie);
> +	if (ring >= bd->rx_nr_rings)
> +		return -EINVAL;
> +
> +	if (flow_type == ETHER_FLOW)
> +		rc = bnge_add_l2_cls_rule(bn, fs);
> +	else
> +		rc = bnge_add_ntuple_cls_rule(bn, cmd);
> +	return rc;
> +}
> +
> +static int bnge_srxclsrldel(struct bnge_net *bn, struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fs = &cmd->fs;
> +	struct bnge_filter_base *fltr_base;
> +	struct bnge_ntuple_filter *fltr;
> +	u32 id = fs->location;
> +
> +	rcu_read_lock();
> +	fltr_base = bnge_get_one_fltr_rcu(bn, bn->l2_fltr_hash_tbl,
> +					  BNGE_L2_FLTR_HASH_SIZE, id, 0);
> +	if (fltr_base) {
> +		struct bnge_l2_filter *l2_fltr;
> +
> +		l2_fltr = container_of(fltr_base, struct bnge_l2_filter, base);
> +		rcu_read_unlock();
> +		bnge_hwrm_l2_filter_free(bn->bd, l2_fltr);
> +		bnge_del_l2_filter_rcu(bn, l2_fltr);
> +		return 0;
> +	}
> +	fltr_base = bnge_get_one_fltr_rcu(bn, bn->ntp_fltr_hash_tbl,
> +					  BNGE_NTP_FLTR_HASH_SIZE, id,
> +					  BNGE_MAX_L2_FLTRS);
> +	if (!fltr_base) {
> +		rcu_read_unlock();
> +		return -ENOENT;
> +	}
> +
> +	fltr = container_of(fltr_base, struct bnge_ntuple_filter, base);
> +	if (!(fltr->base.flags & BNGE_ACT_NO_AGING)) {
> +		rcu_read_unlock();
> +		return -EINVAL;
> +	}
> +	rcu_read_unlock();
> +	bnge_hwrm_cfa_ntuple_filter_free(bn->bd, fltr);
> +	bnge_del_ntp_filter_rcu(bn, fltr);
> +	return 0;
> +}
> +
> +static int bnge_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
> +			  u32 *rule_locs)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +	struct bnge_dev *bd = bn->bd;
> +	int rc = 0;
> +
> +	switch (cmd->cmd) {
> +	case ETHTOOL_GRXCLSRLCNT:
> +		cmd->rule_cnt = bn->user_fltr_count;
> +		cmd->data = bd->max_fltr | RX_CLS_LOC_SPECIAL;
> +		break;
> +
> +	case ETHTOOL_GRXCLSRLALL:
> +		rc = bnge_grxclsrlall(bn, cmd, (u32 *)rule_locs);
> +		break;
> +
> +	case ETHTOOL_GRXCLSRULE:
> +		rc = bnge_grxclsrule(bn, cmd);
> +		break;
> +
> +	default:
> +		rc = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static int bnge_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +	int rc;
> +
> +	switch (cmd->cmd) {
> +	case ETHTOOL_SRXCLSRLINS:
> +		rc = bnge_srxclsrlins(bn, cmd);
> +		break;
> +
> +	case ETHTOOL_SRXCLSRLDEL:
> +		rc = bnge_srxclsrldel(bn, cmd);
> +		break;
> +
> +	default:
> +		rc = -EOPNOTSUPP;
> +		break;
> +	}
> +	return rc;
> +}
> +
>  static const struct ethtool_ops bnge_ethtool_ops = {
>  	.cap_link_lanes_supported	= 1,
>  	.get_link_ksettings	= bnge_get_link_ksettings,

[Severity: Low]
When BNGE_ACT_RSS_CTX is set, this branch sets FLOW_RSS and
cmd->rss_context, but it never writes fs->ring_cookie.
bnge_add_ntuple_cls_rule() still stores the requested ring in base.rxq
for these filters, because the RX_CLS_FLOW_DISC check there doesn't
depend on FLOW_RSS.

ethtool_get_rxnfc() copies the whole struct ethtool_rxnfc from
userspace. So for ETHTOOL_GRXCLSRULE, the ring_cookie reported for an
RSS filter is whatever the caller happened to pass in, not the ring
offset stored with the rule. The drop and plain-ring branches do fill
it in.

The ethtool core rejects a non-zero ring with FLOW_RSS unless
cap_rss_rxnfc_adds is set, and bnge_ethtool_ops doesn't set it. So
base.rxq should always be 0 here, and the practical effect is that the
caller's input gets echoed back. Should the RSS branch also set
fs->ring_cookie = fltr->base.rxq, so the rule reads back the way it
was inserted?

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