Re: [net-next, v4 04/10] bnge: add RXFH ethtool support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 3 · 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] buffer overflow and memset integer underflow when rx_nr_rings >
  512
- [Medium] bnge_set_rxfh_fields() reports success for data == 0 (disable
  hashing) on every flow type except IPV4_FLOW/IPV6_FLOW, but does not…
- [Medium] In bnge_set_rxfh_fields(), the IPv6 branch of the switch
  handles tuple == 4 differently from the IPv4 branch.
- [Medium] The individual AH_V4_FLOW/ESP_V4_FLOW (and
  AH_V6_FLOW/ESP_V6_FLOW) flow types ignore the SPI-hash bits that…
- [Low] In this commit, bnge_set_rxfh() (bnge_ethtool.c) lets userspace
  store a default-context indirection table in bd->rss_indir_tbl while…
- [Low] bnge_ethtool_ops sets .rxfh_per_ctx_key = 1,
  .rxfh_max_num_contexts = BNGE_MAX_ETH_RSS_CTX + 1 and .rxfh_priv_size…
- [Low] The patch changes how the default RSS key is generated, and the
  commit message does not say so.

--

bnge: add RXFH ethtool support

This commit adds ethtool RXFH operations to bnge for reading and
programming the RSS hash key, indirection table and hash fields. The set
operations are only allowed while the interface is down. It also moves
default RSS key generation to netdev_rss_key_fill() in bnge_netdev_alloc()
and adds an IPv6 flow label RSS capability bit.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> index 2467e44de291a..6fbda4fc1a0c4 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c

[ ... ]

> @@ -740,6 +743,272 @@ static int bnge_set_pauseparam(struct net_device *dev,

[ ... ]

> +static int bnge_get_rxfh_fields(struct net_device *dev,
> +				struct ethtool_rxfh_fields *cmd)
> +{

[ ... ]

> +	case AH_ESP_V4_FLOW:
> +		if (bd->rss_hash_cfg &
> +		    (VNIC_RSS_CFG_REQ_HASH_TYPE_AH_SPI_IPV4 |
> +		     VNIC_RSS_CFG_REQ_HASH_TYPE_ESP_SPI_IPV4))
> +			cmd->data |= RXH_IP_SRC | RXH_IP_DST |
> +				     RXH_L4_B_0_1 | RXH_L4_B_2_3;
> +		cmd->data |= bnge_get_ethtool_ipv4_rss(bd);
> +		break;
> +	case SCTP_V4_FLOW:
> +	case AH_V4_FLOW:
> +	case ESP_V4_FLOW:
> +	case IPV4_FLOW:
> +		cmd->data |= bnge_get_ethtool_ipv4_rss(bd);
> +		break;

[Severity: Medium]
Should AH_V4_FLOW and ESP_V4_FLOW report the SPI bits here too?

When VNIC_RSS_CFG_REQ_HASH_TYPE_AH_SPI_IPV4 and ESP_SPI_IPV4 are set, the
hardware hashes AH/ESP packets on the SPI, and AH_ESP_V4_FLOW reports that
as a 4-tuple. AH_V4_FLOW and ESP_V4_FLOW fall into the generic IPv4 case
instead, and only report RXH_IP_SRC | RXH_IP_DST for the same packets.

The set side splits the same way. In bnge_set_rxfh_fields(), only
AH_ESP_V4_FLOW reaches the branch that clears the SPI bits:

	} else if (cmd->flow_type == AH_ESP_V4_FLOW) {
		...
		rss_hash_cfg &= ~(VNIC_RSS_CFG_REQ_HASH_TYPE_AH_SPI_IPV4 |
				  VNIC_RSS_CFG_REQ_HASH_TYPE_ESP_SPI_IPV4);

As a result, a 2-tuple or zero request for AH_V4_FLOW or ESP_V4_FLOW
returns success while SPI hashing stays on.

The ethtool netlink RSS flow-hash attributes expose ah4/esp4 separately
from ah-esp4. An RSS GET can therefore show ah-esp4 and ah4 disagreeing
about the same packets.

The IPv6 cases (AH_V6_FLOW and ESP_V6_FLOW versus AH_ESP_V6_FLOW) look the
same. bnxt_get_rxfh_fields() and bnxt_set_rxfh_fields() follow the same
pattern.

[ ... ]

> +static int bnge_set_rxfh_fields(struct net_device *dev,
> +				const struct ethtool_rxfh_fields *cmd,
> +				struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	switch (cmd->flow_type) {
> +	case TCP_V4_FLOW:
> +	case UDP_V4_FLOW:
> +	case SCTP_V4_FLOW:
> +	case AH_ESP_V4_FLOW:
> +	case AH_V4_FLOW:
> +	case ESP_V4_FLOW:
> +	case IPV4_FLOW:
> +		if (tuple == 2)
> +			rss_hash_cfg |= VNIC_RSS_CFG_REQ_HASH_TYPE_IPV4;
> +		else if (!tuple && cmd->flow_type == IPV4_FLOW)
> +			rss_hash_cfg &= ~VNIC_RSS_CFG_REQ_HASH_TYPE_IPV4;
> +		break;

[Severity: Medium]
What happens when data == 0 is requested for a flow type other than
IPV4_FLOW?

With tuple == 0, VNIC_RSS_CFG_REQ_HASH_TYPE_IPV4 is only cleared for
IPV4_FLOW. For SCTP_V4_FLOW, AH_V4_FLOW and ESP_V4_FLOW nothing changes,
so this check further down returns success, even while the interface is
running:

	if (bd->rss_hash_cfg == rss_hash_cfg)
		return 0;

For TCP/UDP/AH_ESP, only the L4 bit is cleared and the IPv4 2-tuple bit
stays set.

bnge_get_rxfh_fields() then reports RXH_IP_SRC | RXH_IP_DST for a flow type
that was just set to 0, and the hardware keeps hashing those packets. The
default rss_hash_cfg includes VNIC_RSS_CFG_REQ_HASH_TYPE_IPV4, so something
like this takes the no-op path:

  ethtool -N ethX rx-flow-hash sctp4 ""

The IPv6 branch has the same restriction on IPV6_FLOW.

bnxt_set_rxfh_fields() clears the L3 bit for any !tuple request, which is
where the two drivers differ. Would it be better to return an error for
zero requests that can't be applied, instead of returning 0?

> +
> +	case TCP_V6_FLOW:
> +	case UDP_V6_FLOW:
> +	case SCTP_V6_FLOW:
> +	case AH_ESP_V6_FLOW:
> +	case AH_V6_FLOW:
> +	case ESP_V6_FLOW:
> +	case IPV6_FLOW:
> +		if (cmd->data & RXH_IP6_FL) {
> +			rss_hash_cfg |= VNIC_RSS_CFG_REQ_HASH_TYPE_IPV6_FLOW_LABEL;
> +			rss_hash_cfg &= ~VNIC_RSS_CFG_REQ_HASH_TYPE_IPV6;
> +		} else if (tuple == 2 || tuple == 4) {
> +			rss_hash_cfg |= VNIC_RSS_CFG_REQ_HASH_TYPE_IPV6;
> +			rss_hash_cfg &= ~VNIC_RSS_CFG_REQ_HASH_TYPE_IPV6_FLOW_LABEL;

[Severity: Medium]
Is tuple == 4 meant to change the global IPv6 bits here?

On the IPv4 side, a 4-tuple request leaves VNIC_RSS_CFG_REQ_HASH_TYPE_IPV4
alone; only tuple == 2 sets it. Here, a 4-tuple tcp6/udp6/ah-esp6 request
sets VNIC_RSS_CFG_REQ_HASH_TYPE_IPV6 and clears IPV6_FLOW_LABEL.

This sequence turns 2-tuple hashing back on for ip6/sctp6/ah6/esp6:

  ethtool -N ethX rx-flow-hash ip6 ""
  ethtool -N ethX rx-flow-hash tcp6 sdfn

In the same way, a tcp6 sdfn request after ip6 sdl silently turns off flow
label hashing.

RXH_4TUPLE already contains RXH_IP_SRC | RXH_IP_DST, so the TCP_V6_FLOW
readback does not need the IPV6 bit set.

> +		} else if (!tuple && cmd->flow_type == IPV6_FLOW) {

[ ... ]

> +static int bnge_set_rxfh(struct net_device *dev,
> +			 struct ethtool_rxfh_param *rxfh,
> +			 struct netlink_ext_ack *extack)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +
> +	if (rxfh->hfunc && rxfh->hfunc != ETH_RSS_HASH_TOP)
> +		return -EOPNOTSUPP;
> +
> +	if (netif_running(dev)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "RSS configuration can only be changed while the interface is down");
> +		return -EBUSY;
> +	}
> +
> +	bnge_modify_rss(bn, NULL, NULL, rxfh);

[Severity: High]
What limits tbl_size here when bd->rx_nr_rings is large?

bnge_modify_rss() copies bnge_get_rxfh_indir_size(bd) entries into
bd->rss_indir_tbl. That size grows with bd->rx_nr_rings through
bnge_cal_nr_rss_ctxs(). The table itself only holds
bd->rss_indir_tbl_entries entries (BNGE_MAX_RSS_TABLE_ENTRIES, 512):

drivers/net/ethernet/broadcom/bnge/bnge_vnic.c:bnge_modify_rss() {
    ...
		for (i = 0; i < tbl_size; i++)
			indir_tbl[i] = rxfh->indir[i];
		pad = bd->rss_indir_tbl_entries - tbl_size;
		if (pad && !rss_ctx)
			memset(&indir_tbl[i], 0, pad * sizeof(*indir_tbl));
    ...
}

With more than 512 RX rings, tbl_size is larger than the table. The
copy loop then writes past the end of bd->rss_indir_tbl. pad is a u32,
so it wraps around to a value near 4G, and the memset() clears
gigabytes of kernel memory after the table.

The ethtool core sizes rxfh->indir from get_rxfh_indir_size, so the
user buffer is fine. It is the driver table that overflows.
bnge_get_rxfh() has the matching out-of-bounds read, because it
copies tbl_size entries out of bd->rss_indir_tbl.

bnge_fill_hw_rss_tbl() uses the same tbl_size. It writes two __le16
values per entry into vnic->rss_table, which would overrun that DMA
buffer on open. That code predates this patch, but the root cause is
the same.

Does anything in the ring reservation path keep rx_nr_rings at or
below BNGE_MAX_RSS_TABLE_ENTRIES? If not, should the RX ring count be
capped there, or should bnge_get_rxfh_indir_size() be clamped to
bd->rss_indir_tbl_entries?

[Severity: Low]
Does the indirection table stored here survive the next open?

bnge_open_core() calls bnge_reserve_rings(), which still resets the table
without checking netif_is_rxfh_configured():

drivers/net/ethernet/broadcom/bnge/bnge_resc.c:bnge_reserve_rings() {
    ...
	if (old_rx_rings != bd->hw_resc.resv_rx_rings)
		bnge_set_dflt_rss_indir_tbl(bd);
    ...
}

The reserved RX ring count can change while the interface is down. For
example, bnge_is_agg_reqd() can flip after an MTU or LRO/GRO change, or
firmware can grant a different number of RX rings. In that case the user
table is silently overwritten while IFF_RXFH_CONFIGURED stays set.

The next patch in the series, "bnge: add ethtool support to manage RSS
contexts", appears to add a netif_is_rxfh_configured() guard and
ethtool_rxfh_indir_lost() handling here. If so, only this intermediate
commit is affected.

[ ... ]

> @@ -757,6 +1026,18 @@ static const struct ethtool_ops bnge_ethtool_ops = {
>  	.get_eth_ctrl_stats	= bnge_get_eth_ctrl_stats,
>  	.get_pause_stats	= bnge_get_pause_stats,
>  	.get_rmon_stats		= bnge_get_rmon_stats,
> +	/* RXFH */
> +	.rxfh_per_ctx_key	= 1,
> +	.rxfh_max_num_contexts	= BNGE_MAX_ETH_RSS_CTX + 1,
> +	.rxfh_indir_space	= BNGE_MAX_RSS_TABLE_ENTRIES,
> +	.rxfh_priv_size		= sizeof(struct bnge_rss_ctx),

[Severity: Low]
This isn't a bug, but rxfh_per_ctx_key, rxfh_max_num_contexts and
rxfh_priv_size are set here without create_rxfh_context,
modify_rxfh_context or remove_rxfh_context. At this commit the ethtool core
rejects every context request with -EOPNOTSUPP, so these fields and
BNGE_MAX_ETH_RSS_CTX have no effect.

The next patch, "bnge: add ethtool support to manage RSS contexts", adds
the callbacks. Would these fields fit better in that patch?

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index c6b9048586b36..ef5d13337e60b 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -1739,13 +1739,6 @@ static void bnge_init_vnics(struct bnge_net *bn)
>  				u8 *key = (void *)vnic->rss_hash_key;
>  				int k;
>  
> -				if (!bn->rss_hash_key_valid &&
> -				    !bn->rss_hash_key_updated) {
> -					get_random_bytes(bn->rss_hash_key,
> -							 HW_HASH_KEY_SIZE);
> -					bn->rss_hash_key_updated = true;
> -				}
> -
>  				memcpy(vnic->rss_hash_key, bn->rss_hash_key,
>  				       HW_HASH_KEY_SIZE);
>  
> @@ -3398,6 +3391,9 @@ int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
>  	bn->tx_ring_size = BNGE_DEFAULT_TX_RING_SIZE;
>  	bn->rx_dir = DMA_FROM_DEVICE;
>  
> +	netdev_rss_key_fill(bn->rss_hash_key, sizeof(bn->rss_hash_key));
> +	bn->rss_hash_key_updated = true;
> +

[Severity: Low]
This isn't a bug, but the commit message doesn't mention this change to
the default RSS key.

netdev_rss_key_fill() copies the global netdev_rss_key, so all bnge ports
now share one key. Before, each port got its own get_random_bytes() key on
first open.

Also, the removed check in bnge_init_vnics() was the only reader of
bn->rss_hash_key_valid. bnge_init_vnics() still sets it:

	bn->rss_hash_key_valid = true;

but nothing tests it any more. Can that field be removed?

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