Re: [PATCH v2 1/4] net/nbl: fix memzone leak in queue release

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 25 Jan 2026 23:58:12 -0800
Dimon Zhao <dimon.zhao@nebula-matrix.com> wrote:

> diff --git a/drivers/net/nbl/nbl_hw/nbl_txrx.c b/drivers/net/nbl/nbl_hw/nbl_txrx.c
> index 650790b4fc..563f011cd3 100644
> --- a/drivers/net/nbl/nbl_hw/nbl_txrx.c
> +++ b/drivers/net/nbl/nbl_hw/nbl_txrx.c
> @@ -69,9 +69,10 @@ static void nbl_res_txrx_release_tx_ring(void *priv, u16 queue_idx)
>  	struct nbl_resource_mgt *res_mgt = (struct nbl_resource_mgt *)priv;
>  	struct nbl_txrx_mgt *txrx_mgt = NBL_RES_MGT_TO_TXRX_MGT(res_mgt);
>  	struct nbl_res_tx_ring *tx_ring = NBL_RES_MGT_TO_TX_RING(res_mgt, queue_idx);
> -	if (!tx_ring)
> -		return;
> +

Automated patch review spotted this removal as possible problem.

Code Review:

ERROR: NULL pointer dereference introduced

The NULL check for tx_ring was removed but the code immediately dereferences it:
c

static void nbl_res_txrx_release_tx_ring(void *priv, u16 queue_idx)
{
	struct nbl_resource_mgt *res_mgt = (struct nbl_resource_mgt *)priv;
	struct nbl_txrx_mgt *txrx_mgt = NBL_RES_MGT_TO_TXRX_MGT(res_mgt);
	struct nbl_res_tx_ring *tx_ring = NBL_RES_MGT_TO_TX_RING(res_mgt, queue_idx);

	rte_free(tx_ring->tx_entry);  /* <-- crash if tx_ring is NULL */

The old code had:
c

	if (!tx_ring)
		return;

This guard must be restored, or the callers must guarantee tx_ring is never NULL.

Similarly for nbl_res_txrx_release_rx_ring — no NULL check before dereferencing rx_ring->rx_entry and rx_ring->desc_mz.



Further AI lead analysis show scenarios where it could be a problem.

Complete Call Chain Analysis for Patch 1/4
Call Flow

DPDK Application
    ↓
rte_eth_dev_tx_queue_release() or eth_dev_ops->tx_queue_release
    ↓
nbl_tx_queues_release() [nbl_dev.c:420]
    ↓
disp_ops->release_tx_ring() → nbl_disp_release_tx_ring() [nbl_dispatch.c:163]
    ↓
res_ops->release_tx_ring() → nbl_res_txrx_release_tx_ring() [nbl_txrx.c]

Where tx_ring Can Be NULL

The macro definition:
c

#define NBL_RES_MGT_TO_TX_RING(res_mgt, index)	\
    (NBL_RES_MGT_TO_TXRX_MGT(res_mgt)->tx_rings[(index)])

Where tx_rings is:
c

struct nbl_res_tx_ring **tx_rings;  // Array of pointers

Scenario 1: Normal close without all queues set up

In nbl_dev_port_close() → nbl_release_queues() (line 337-349):
c

for (i = 0; i < eth_dev->data->nb_tx_queues; i++)
    disp_ops->release_tx_ring(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt), i);

If the application configured 4 queues but only called tx_queue_setup() on 2 of them, tx_rings[2] and tx_rings[3] will be NULL.

Scenario 2: DPDK ethdev layer calling release on never-setup queue

DPDK's rte_eth_dev_close() can call tx_queue_release for all configured queues without validating they were set up.

Scenario 3: Re-setup path mismatch

In the patched nbl_res_txrx_start_tx_ring():
c

if (eth_dev->data->tx_queues[param->queue_idx] != NULL) {
    NBL_LOG(WARNING, "re-setup an already allocated tx queue");
    nbl_res_txrx_release_tx_ring(priv, param->queue_idx);  // <-- Called

The check is on eth_dev->data->tx_queues[] (ethdev layer), but tx_rings[] (resource layer) could differ if a previous setup partially failed.
Verdict: Confirmed Bug in Patch 1/4

The removal of NULL checks causes NULL pointer dereference in both nbl_res_txrx_release_tx_ring() and nbl_res_txrx_release_rx_ring().
Required Fix
c

static void nbl_res_txrx_release_tx_ring(void *priv, u16 queue_idx)
{
    struct nbl_resource_mgt *res_mgt = (struct nbl_resource_mgt *)priv;
    struct nbl_txrx_mgt *txrx_mgt = NBL_RES_MGT_TO_TXRX_MGT(res_mgt);
    struct nbl_res_tx_ring *tx_ring = NBL_RES_MGT_TO_TX_RING(res_mgt, queue_idx);

+   if (!tx_ring)
+       return;
+
    rte_free(tx_ring->tx_entry);
    rte_memzone_free(tx_ring->net_hdr_mz);
    rte_memzone_free(tx_ring->desc_mz);
    rte_free(tx_ring);
    txrx_mgt->tx_rings[queue_idx] = NULL;
}

static void nbl_res_txrx_release_rx_ring(void *priv, u16 queue_idx)
{
    struct nbl_resource_mgt *res_mgt = (struct nbl_resource_mgt *)priv;
    struct nbl_txrx_mgt *txrx_mgt = NBL_RES_MGT_TO_TXRX_MGT(res_mgt);
    struct nbl_res_rx_ring *rx_ring = NBL_RES_MGT_TO_RX_RING(res_mgt, queue_idx);

+   if (!rx_ring)
+       return;
+
    rte_free(rx_ring->rx_entry);
    rte_memzone_free(rx_ring->desc_mz);
    rte_free(rx_ring);
    txrx_mgt->rx_rings[queue_idx] = NULL;
}

The memzone leak fix itself is correct and needed — but the defensive NULL checks must be preserved.

Conclusion put the null checks back into rx and tx and retest/resubmit new version.


Full chat is here if you are interested: https://claude.ai/share/d543bddd-7406-4f00-b5cb-4a92e8718476