|SUCCESS| [v2, 4/4] net/axgbe: fix descriptor status out-of-bounds access

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163807

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-08

I'll review this DPDK patch series focusing on correctness, resource management, coding style, and guideline compliance.

Commit: 20260508191109.734377-2
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 1/4] net/axgbe: fix resource leaks in device init error paths

This patch addresses resource leaks in eth_axgbe_dev_init() by adding
proper cleanup code to multiple error paths. The changes include freeing
mac_addrs and hash_mac_addrs on failures, checking
rte_intr_callback_register() return value, and handling hw_if.exit()
errors properly.

> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index cfcd880961..709a52acc8 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -2359,6 +2359,8 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>  		PMD_INIT_LOG(ERR,
>  			     "Failed to allocate %d bytes needed to "
>  			     "store MAC addresses", len);
> +		rte_free(eth_dev->data->mac_addrs);
> +		eth_dev->data->mac_addrs = NULL;
>  		return -ENOMEM;
>  	}

[ ... ]

> @@ -2405,8 +2407,14 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>  					  pdata->vdata->rx_max_fifo_size);
>  	/* Issue software reset to DMA */
>  	ret = pdata->hw_if.exit(pdata);
> -	if (ret)
> +	if (ret) {
>  		PMD_DRV_LOG_LINE(ERR, "hw_if->exit EBUSY error");
> +		rte_free(eth_dev->data->hash_mac_addrs);
> +		eth_dev->data->hash_mac_addrs = NULL;
> +		rte_free(eth_dev->data->mac_addrs);
> +		eth_dev->data->mac_addrs = NULL;
> +		return ret;
> +	}

[ ... ]

> @@ -2426,14 +2434,23 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>
>  	ret = pdata->phy_if.phy_init(pdata);
>  	if (ret) {
> +		rte_free(eth_dev->data->hash_mac_addrs);
> +		eth_dev->data->hash_mac_addrs = NULL;
>  		rte_free(eth_dev->data->mac_addrs);
>  		eth_dev->data->mac_addrs = NULL;
>  		return ret;
>  	}
>
> -	rte_intr_callback_register(pci_dev->intr_handle,
> +	ret = rte_intr_callback_register(pci_dev->intr_handle,
>  				   axgbe_dev_interrupt_handler,
>  				   (void *)eth_dev);
> +	if (ret) {
> +		rte_free(eth_dev->data->hash_mac_addrs);
> +		eth_dev->data->hash_mac_addrs = NULL;
> +		rte_free(eth_dev->data->mac_addrs);
> +		eth_dev->data->mac_addrs = NULL;
> +		return ret;
> +	}
>  	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x",
>  		     eth_dev->data->port_id, pci_dev->id.vendor_id,
>  		     pci_dev->id.device_id);

When rte_intr_callback_register() fails in eth_axgbe_dev_init(), does
this code need to unwind the phy_init() work? After phy_init() succeeds,
if interrupt registration fails, does this leave hardware in an
initialized state without proper cleanup? Should this error path also
call any corresponding phy cleanup function?


Commit: 20260508191109.734377-3
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 2/4] net/axgbe: fix Rx queue leak on descriptor init failure

This patch replaces individual rte_mbuf_raw_alloc() calls with
rte_pktmbuf_alloc_bulk() and adds proper cleanup of previously
initialized queues on failure.

> diff --git a/drivers/net/axgbe/axgbe_dev.c b/drivers/net/axgbe/axgbe_dev.c
> index 482d3d8062..9eabe0dfa8 100644
> --- a/drivers/net/axgbe/axgbe_dev.c
> +++ b/drivers/net/axgbe/axgbe_dev.c
> @@ -1011,13 +1011,11 @@ static void wrapper_tx_desc_init(struct axgbe_port *pdata)
>
>  static int wrapper_rx_desc_init(struct axgbe_port *pdata)
>  {
> -	struct axgbe_rx_queue *rxq;
> -	struct rte_mbuf *mbuf;
>  	volatile union axgbe_rx_desc *desc;
>  	unsigned int i, j;
>
>  	for (i = 0; i < pdata->eth_dev->data->nb_rx_queues; i++) {
> -		rxq = pdata->eth_dev->data->rx_queues[i];
> +		struct axgbe_rx_queue *rxq = pdata->eth_dev->data->rx_queues[i];
>
>  		/* Initialize software ring entries */
>  		rxq->mbuf_alloc = 0;
> @@ -1025,19 +1023,18 @@ static int wrapper_rx_desc_init(struct axgbe_port *pdata)
>  		rxq->dirty = 0;
>  		desc = AXGBE_GET_DESC_PT(rxq, 0);
>
> +		if (!rte_pktmbuf_alloc_bulk(rxq->mb_pool, rxq->sw_ring, rxq->nb_desc)) {

Does the return value check here have the correct logic? Looking at
rte_pktmbuf_alloc_bulk() semantics, when does this function return 0
versus non-zero? The condition !rte_pktmbuf_alloc_bulk() suggests
entering the error path when the function returns 0, but typically bulk
allocation functions return 0 on success. Can this be verified against
the DPDK API documentation?

> +			PMD_DRV_LOG_LINE(ERR, "RX mbuf alloc failed queue_id = %u, nb_desc = %u",
> +					 i, rxq->nb_desc);
> +			for (unsigned int k = 0; k < i; k++)
> +				axgbe_dev_rx_queue_release(pdata->eth_dev, k);
> +			return -ENOMEM;
> +		}
> +
>  		for (j = 0; j < rxq->nb_desc; j++) {
> -			mbuf = rte_mbuf_raw_alloc(rxq->mb_pool);
> -			if (mbuf == NULL) {
> -				PMD_DRV_LOG_LINE(ERR, "RX mbuf alloc failed queue_id = %u, idx = %d",
> -					    (unsigned int)rxq->queue_id, j);
> -				axgbe_dev_rx_queue_release(pdata->eth_dev, i);
> -				return -ENOMEM;
> -			}
> -			rxq->sw_ring[j] = mbuf;
> -			/* Mbuf populate */
> -			mbuf->next = NULL;
> -			mbuf->data_off = RTE_PKTMBUF_HEADROOM;
> -			mbuf->nb_segs = 1;
> +			struct rte_mbuf *mbuf = rxq->sw_ring[j];
> +
> +			/* mbuf is in reset state (nb_segs = 1, headroom, etc)  */
>  			mbuf->port = rxq->port_id;
>  			desc->read.baddr =
>  				rte_cpu_to_le_64(

The comment states mbufs are in reset state, but this changes the
behavior from using rte_mbuf_raw_alloc() to rte_pktmbuf_alloc_bulk().
Does rte_pktmbuf_alloc_bulk() provide the same guarantees as
rte_mbuf_raw_alloc() regarding mbuf initialization? The previous code
explicitly set next, data_off, and nb_segs fields after
rte_mbuf_raw_alloc(), but these assignments are removed. Does this
assume rte_pktmbuf_alloc_bulk() already initializes these fields, and
does this match the original code's expectations?


Commit: 20260508191109.734377-4
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 3/4] net/axgbe: destroy mutexes on device close

> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index 709a52acc8..c8414998b4 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -2482,6 +2482,11 @@ axgbe_dev_close(struct rte_eth_dev *eth_dev)
>  	/* Disable all interrupts in the hardware */
>  	XP_IOWRITE(pdata, XP_INT_EN, 0x0);
>
> +	pthread_mutex_destroy(&pdata->xpcs_mutex);
> +	pthread_mutex_destroy(&pdata->i2c_mutex);
> +	pthread_mutex_destroy(&pdata->an_mutex);
> +	pthread_mutex_destroy(&pdata->phy_mutex);
> +
>  	return 0;
>  }

The mutexes are destroyed in axgbe_dev_close(), but the patch doesn't
address whether these mutexes could be in use (locked) at this point. In
the error paths added to eth_axgbe_dev_init() in patch 1/4, when init
fails before reaching the end, do any code paths leave these mutexes
locked? Does axgbe_dev_close()->pthread_mutex_destroy() need to check
lock state before destroying?


Commit: 20260508191109.734377-5
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 4/4] net/axgbe: fix descriptor status out-of-bounds access

> diff --git a/drivers/net/axgbe/axgbe_rxtx.c b/drivers/net/axgbe/axgbe_rxtx.c
> index 51a1aeb0b9..6f750d6ede 100644
> --- a/drivers/net/axgbe/axgbe_rxtx.c
> +++ b/drivers/net/axgbe/axgbe_rxtx.c
> @@ -1205,8 +1205,8 @@ axgbe_dev_rx_descriptor_status(void *rx_queue, uint16_t offset)
>  	if (offset >= rxq->nb_desc - rxq->dirty)
>  		return RTE_ETH_RX_DESC_UNAVAIL;
>
> -	idx = AXGBE_GET_DESC_IDX(rxq, rxq->cur);
> -	desc = &rxq->desc[idx + offset];
> +	idx = AXGBE_GET_DESC_IDX(rxq, rxq->cur + offset);
> +	desc = &rxq->desc[idx];

[ ... ]

> @@ -1228,8 +1228,8 @@ axgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t offset)
>  	if (offset >= txq->nb_desc - txq->dirty)
>  		return RTE_ETH_TX_DESC_UNAVAIL;
>
> -	idx = AXGBE_GET_DESC_IDX(txq, txq->dirty + txq->free_batch_cnt - 1);
> -	desc = &txq->desc[idx + offset];
> +	idx = AXGBE_GET_DESC_IDX(txq, txq->dirty + txq->free_batch_cnt - 1 + offset);
> +	desc = &txq->desc[idx];

The arithmetic txq->dirty + txq->free_batch_cnt - 1 + offset involves
unsigned integer addition that could overflow before AXGBE_GET_DESC_IDX
applies the modulo operation. Does this arithmetic overflow issue matter
given that AXGBE_GET_DESC_IDX masks the result? Would it be clearer to
break this into separate operations to avoid potential undefined behavior
in the intermediate computation?