Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
bnxt_queue_up: free completion ring on failure
To:
tech@openbsd.org
Cc:
jmatthew@openbsd.org
Date:
Tue, 7 Oct 2025 15:34:56 +0200

Download raw body.

Thread
If bnxt firmware fails to allocate a Tx ring in bnxt_queue_up()
then we jump into an error path which leaks a completion ring.

Found while running ping traffic across bnxt0 in parallel with:

  while true; do ifconfig bnxt0 down up; done

The patch below avoids leaking a completion ring but it does not
resolve the problem where the above down/up loop eventually fails:

  ifconfig: bnxt0: SIOCSIFFLAGS: Cannot allocate memory

  bnxt0: HWRM_RING_ALLOC command returned RESOURCE_ALLOC_ERROR error.
  bnxt0: failed to set up tx ring

There might be another resource leak caused by the driver, or perhaps
firmware is leaking memory and will eventually always hit this problem.

bnxt0 at pci14 dev 0 function 0 "Broadcom BCM57414" rev 0x01: fw ver 234.0.150, msix, 8 queues, address xx:xx:xx:xx:xx:xx


ok?

free completion ring on failure in bnxt_queue_up()

M  sys/dev/pci/if_bnxt.c  |  13+  3-

1 file changed, 13 insertions(+), 3 deletions(-)

commit - dfd988efd85f876f66c82e5f98ff3ff8ac9ed907
commit + bdb26caee92dfd55bf4a07741f3e3b15cf02cfc7
blob - 49b4a418a8801b88632d1a78fe29d589c5188f92
blob + 87a8f220796fc433431e49a7484e81e4b8ebd50e
--- sys/dev/pci/if_bnxt.c
+++ sys/dev/pci/if_bnxt.c
@@ -821,13 +821,13 @@ bnxt_queue_up(struct bnxt_softc *sc, struct bnxt_queue
 		    HWRM_NA_SIGNATURE, 1) != 0) {
 			printf("%s: failed to allocate completion queue %d\n",
 			    DEVNAME(sc), bq->q_index);
-			goto free_rx;
+			goto free_cp_ring_mem;
 		}
 
 		if (bnxt_set_cp_ring_aggint(sc, cp) != 0) {
 			printf("%s: failed to set interrupt %d aggregation\n",
 			    DEVNAME(sc), bq->q_index);
-			goto free_rx;
+			goto free_cp_ring;
 		}
 		bnxt_write_cp_doorbell(sc, &cp->ring, 1);
 	}
@@ -836,7 +836,7 @@ bnxt_queue_up(struct bnxt_softc *sc, struct bnxt_queue
 	    BNXT_DMA_DVA(sc->sc_stats_ctx_mem) +
 	    (bq->q_index * sizeof(struct ctx_hw_stats))) != 0) {
 		printf("%s: failed to set up stats context\n", DEVNAME(sc));
-		goto free_rx;
+		goto free_cp_ring;
 	}
 
 	tx->tx_ring.phys_id = (uint16_t)HWRM_NA_SIGNATURE;
@@ -998,6 +998,16 @@ dealloc_rx:
 	    &rx->rx_ring);
 dealloc_stats:
 	bnxt_hwrm_stat_ctx_free(sc, cp);
+free_cp_ring:
+	if (sc->sc_intrmap != NULL) {
+		bnxt_hwrm_ring_free(sc,
+		    HWRM_RING_ALLOC_INPUT_RING_TYPE_L2_CMPL, &cp->ring);
+	}
+free_cp_ring_mem:
+	if (sc->sc_intrmap != NULL) {
+		bnxt_dmamem_free(sc, cp->ring_mem);
+		cp->ring_mem = NULL;
+	}
 free_rx:
 	bnxt_dmamem_free(sc, rx->rx_ring_mem);
 	rx->rx_ring_mem = NULL;