Re: [PATCH net-next v3 7/7] bnxt_en: Fix ring accounting and validation when rings are constrained

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (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] bnxt_reserve_rings() has a new branch, `else if (irq_re_init &&
  BNXT_NEW_RM(bp) && !rc)`, that can call bnxt_change_msix() while…
- [Medium] In bnxt_reserve_rings()'s callees, __bnxt_reserve_rings() and
  bnxt_init_int_mode(), bnxt_trim_rings() still runs before…

Pre-existing issues:
- [High] The patch adds a new way for RX rings to shrink, and that path
  reaches an existing out-of-bounds access.

--

bnxt_en: Fix ring accounting and validation when rings are constrained

This patch renames bnxt_adj_tx_rings() to bnxt_adj_rings(). TX, RX and CP
rings are now scaled down together for XDP, TCs and shared rings, and
bnxt_rings_ok() now checks for a minimum viable ring layout. The
adjustment is centralized in __bnxt_reserve_rings() and
bnxt_init_int_mode(), and bnxt_reserve_rings() recomputes the required
IRQs after the rings may have shrunk.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index ed3f67dea0306..cbf7aef1d5b82 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -11530,19 +11573,50 @@ static int bnxt_init_int_mode(struct bnxt *bp)
>  		bp->irq_tbl[i].vector = pci_irq_vector(bp->pdev, i);
>  
>  	bp->total_irqs = total_vecs;
> +	req_tx_rings = bp->tx_nr_rings;
> +	req_tx_per_tc = bp->tx_nr_rings_per_tc;
> +	req_tx_xdp = bp->tx_nr_rings_xdp;
> +	req_rx_rings = bp->rx_nr_rings;
> +	req_cp_rings = bp->cp_nr_rings;
> +
>  	/* Trim rings based upon num of vectors allocated */
>  	rc = bnxt_trim_rings(bp, &bp->rx_nr_rings, &bp->tx_nr_rings,
>  			     total_vecs - ulp_msix, min == 1);
>  	if (rc)
>  		goto msix_setup_exit;
>  
> -	tx_cp = bnxt_num_tx_to_cp(bp, bp->tx_nr_rings);
> -	bp->cp_nr_rings = (min == 1) ?
> -			  max_t(int, tx_cp, bp->rx_nr_rings) :
> -			  tx_cp + bp->rx_nr_rings;
> +	min_tx = bp->tx_nr_rings_xdp ? tcs + 1 : tcs;
> +	if (bp->tx_nr_rings < min_tx) {
> +		netdev_err(bp->dev, "Not enough MSI-X to satisfy min. TX rings\n");
> +		rc = -ENOMEM;
> +		goto msix_setup_exit_restore;
> +	}
> +
> +	bnxt_adj_rings(bp);

[Severity: Medium]
Can the TX count returned by bnxt_trim_rings() be negative or too small
here when XDP and mqprio TCs are both in use?

bnxt_trim_rings() still runs before bnxt_adj_rings(). When it trims the CP
count, it converts back to a TX count with:

bnxt_num_cp_to_tx() {
	int tcs = bp->num_tc;

	return (tx_cp - bp->tx_nr_rings_xdp) * tcs +
	       bp->tx_nr_rings_xdp;
}

This uses the untrimmed tx_nr_rings_xdp. Take num_tc=2 with XDP,
per_tc=8, xdp=8 and tx=24, so tx_cp is 16. With a limit of 5, tx becomes
(5 - 8) * 2 + 8 = 2. That is below min_tx = 3, so this returns -ENOMEM.

The layout per_tc=2, xdp=2, tx=6, rx=2 needs only cp = max(4, 2) = 4,
which fits in 5.

The result is also not monotonic. A limit of 6 gives tx=4, which
bnxt_adj_rings() turns into a valid per_tc=1 layout, but limits of 5
and 4 both fail.

__bnxt_reserve_rings() has the same trim. The hwr.tx it produces is then
rejected by the new bnxt_rings_ok() check.

The commit message says:

    It will now correctly scale down TX rings for XDP and TCs evenly.

Should the trim path also scale the XDP rings down, rather than failing
with -ENOMEM when a smaller valid layout exists?

> +
> +	if (bp->bnapi && (req_tx_rings != bp->tx_nr_rings ||
> +			  req_rx_rings != bp->rx_nr_rings)) {

[ ... ]

> @@ -11590,7 +11662,6 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  	irqs_required = bnxt_irqs_required(bp);
>  
>  	if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) {
> -		irq_change = true;
>  		if (!pci_msix_can_alloc_dyn(bp->pdev) || !bp->irq_tbl) {
>  			bnxt_ulp_irq_stop(bp);
>  			bnxt_clear_int_mode(bp);
> @@ -11602,25 +11673,23 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  		if (!rc)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
> -	} else if (irq_change && !rc) {
> -		if (bnxt_change_msix(bp, irqs_required) != irqs_required)
> -			rc = -ENOSPC;
> +	} else if (irq_re_init && BNXT_NEW_RM(bp) && !rc) {
> +		/* __bnxt_reserve_rings() may have shrunk the rings */
> +		irqs_required = bnxt_irqs_required(bp);
> +		if (irqs_required != bp->total_irqs) {
> +			/* Don't bother reducing static IRQs to avoid disrupting
> +			 * RoCE.
> +			 */
> +			if (pci_msix_can_alloc_dyn(bp->pdev) &&
> +			    bnxt_change_msix(bp, irqs_required) !=
> +			    irqs_required)
> +				rc = -ENOSPC;

[Severity: High]
Can this new branch be reached while bp->irq_tbl is NULL?

bnxt_clear_int_mode() frees the vectors and irq_tbl but does not reset
bp->total_irqs:

bnxt_clear_int_mode() {
	pci_free_irq_vectors(bp->pdev);

	kfree(bp->irq_tbl);
	bp->irq_tbl = NULL;
	bp->irq_tbl_size = 0;
}

If the ring config has not changed, irqs_required still equals the stale
bp->total_irqs at the first check, so irq_cleared stays false.

__bnxt_reserve_rings() can then shrink the rings, since bnapi is NULL and
bnxt_adj_rings() runs. After that, the recomputed irqs_required differs
from total_irqs and this calls bnxt_change_msix(), which returns early:

bnxt_change_msix() {
	if (!bp->irq_tbl)
		return 0;
	...
}

So rc becomes -ENOSPC whenever pci_msix_can_alloc_dyn() is true.

Before this patch, irq_change was set only when the first comparison
mismatched. With a NULL irq_tbl, that always took the irq_cleared path,
so bnxt_change_msix() was never called without a table.

Two paths appear to reach this state. The first is AER recovery:

bnxt_io_slot_reset()
    bnxt_clear_int_mode()
    (reservations cleared)
bnxt_io_resume()
    err = bnxt_reserve_rings(bp, true);
    if (!err && !bp->irq_tbl)
        err = bnxt_init_int_mode(bp);

The second is bnxt_hwrm_if_change() on a FW reset or caps change. It
calls bnxt_clear_int_mode() and bnxt_cancel_reservations() before
__bnxt_open_nic()->bnxt_reserve_rings(bp, true).

If firmware grants fewer rings after the reset, does bnxt_io_resume()
skip netif_device_attach() and leave the device detached? On the FW
reset path, does bnxt_open() fail?

Would adding a bp->irq_tbl check to this condition, or resetting
bp->total_irqs in bnxt_clear_int_mode(), avoid this?

> +		}
>  	}

[ ... ]

> @@ -13118,13 +13187,29 @@ static void bnxt_set_xdp_tx_rings(struct bnxt *bp)
>  	bp->tx_nr_rings += bp->tx_nr_rings_xdp;
>  }
>  
> -static void bnxt_adj_tx_rings(struct bnxt *bp)
> +static void bnxt_adj_rings(struct bnxt *bp)
>  {
> -	/* Make adjustments if reserved TX rings are less than requested */
> -	bp->tx_nr_rings -= bp->tx_nr_rings_xdp;
> -	bp->tx_nr_rings_per_tc = bnxt_tx_nr_rings_per_tc(bp);
> -	if (bp->tx_nr_rings_xdp)
> -		bnxt_set_xdp_tx_rings(bp);
> +	int tcs = bp->num_tc ? bp->num_tc : 1;
> +	int rx = bp->rx_nr_rings;
> +
> +	/* Make adjustments to rings based on TC/XDP/shared rings policies */
> +	if (bp->tx_nr_rings_xdp) {
> +		tcs++;
> +		bp->tx_nr_rings_per_tc = bp->tx_nr_rings / tcs;
> +		bp->tx_nr_rings_xdp = bp->tx_nr_rings_per_tc;
> +	} else {
> +		bp->tx_nr_rings_per_tc = bnxt_tx_nr_rings_per_tc(bp);
> +	}
> +
> +	if (bp->flags & BNXT_FLAG_SHARED_RINGS) {
> +		bp->tx_nr_rings_per_tc = min(bp->tx_nr_rings_per_tc, rx);
> +		bp->rx_nr_rings = bp->tx_nr_rings_per_tc;

[Severity: High]
This is a pre-existing issue, but this clamp is a new way for the RX
rings to shrink, and it leads to an existing out-of-bounds access through
non-default RSS contexts.

In shared mode, a shortfall in TX alone now reduces RX. For example, with
TCs=2, 8 combined channels, and FW granting 8 of 16 TX rings, RX drops
from 8 to 4. The old bnxt_adj_tx_rings() left RX unchanged in this case.

bnxt_check_rss_tbl_lost() and bnxt_get_max_rss_ring() only look at the
default bp->rss_indir_tbl. Additional RSS contexts created with
"ethtool -X ... context N" keep their old indices.

On the next open, bnxt_alloc_mem() sizes bp->rx_ring[] from the smaller
rx_nr_rings, and then:

bnxt_hwrm_realloc_rss_ctx_vnic()
  __bnxt_setup_vnic_p5()
    bnxt_fill_hw_rss_tbl_p5()
		else if (vnic->flags & BNXT_VNIC_RSSCTX_FLAG)
			j = ethtool_rxfh_context_indir(vnic->rss_ctx)[i];
		...
		rxr = &bp->rx_ring[j];

		ring_id = rxr->rx_ring_struct.fw_ring_id;
		*ring_tbl++ = cpu_to_le16(ring_id);
		ring_id = bnxt_cp_ring_for_rx(bp, rxr);

Does this read past the end of bp->rx_ring[] on P5+ chips? It would then
dereference rxr->rx_cpr taken from out-of-bounds memory and write garbage
ring IDs into the hardware RSS table.

The commit message says the RSS context and n-tuple filter problems on RX
shrink will be fixed in a separate patchset. This patch makes the shrink
reachable in TX-constrained combined-mode setups that used to keep RX
intact. Should that fix land together with this one?

> +		if (bp->tx_nr_rings_xdp)
> +			bp->tx_nr_rings_xdp = bp->tx_nr_rings_per_tc;
> +	}
> +
> +	bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tcs;
> +	bnxt_set_cp_rings(bp, bp->flags & BNXT_FLAG_SHARED_RINGS);
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012220827.340998-1-michael.chan%40broadcom.com