|WARNING| net/idpf: fix Tx payload corruption in split queue

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170328

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-30

# DPDK Patch Review

## Summary
This patch fixes a critical use-after-free bug in the idpf driver's split-queue Tx completion path. The fix introduces a shadow completion ring to decouple mbuf lifetime from descriptor ring slot reuse. Overall, the approach is sound, but there are several issues that need to be addressed.

---

## Errors

### 1. Resource leak on allocation failure
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Line:** 238-246

The function `idpf_qc_split_tx_descq_reset()` allocates `tx_pending_pkts` but returns `void`, so the allocation failure cannot be propagated to the caller. The caller will proceed as if initialization succeeded, leading to NULL pointer dereference when the Tx path tries to use `tx_pending_pkts`.

**Fix:** Either change the function signature to return `int` and propagate the error, or fail more gracefully (e.g., mark the queue as unusable). Since this is a reset function likely called from queue setup, returning an error is preferred:

```c
int
idpf_qc_split_tx_descq_reset(struct ci_tx_queue *txq)
{
	/* ... existing code ... */
	
	if (txq->tx_pending_pkts == NULL) {
		uint32_t pending_sz = rte_align32pow2(txq->nb_tx_desc);

		txq->tx_pending_pkts = rte_zmalloc_socket("idpf_tx_pending",
			sizeof(struct rte_mbuf *) * pending_sz,
			RTE_CACHE_LINE_SIZE, SOCKET_ID_ANY);
		if (txq->tx_pending_pkts == NULL) {
			DRV_LOG(ERR, "Failed to alloc idpf tx_pending shadow ring");
			return -ENOMEM;
		}
		txq->tx_pending_mask = (uint16_t)(pending_sz - 1);
	} else {
		/* ... */
	}
	txq->tx_next_compl_tag = 0;
	return 0;
}
```

Then update all call sites to check the return value.

### 2. Variable `i` used without declaration in reset function
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Line:** 249

The variable `i` is used in the loop `for (i = 0; i <= txq->tx_pending_mask; i++)` but is never declared in the function scope. This will cause a compilation failure.

**Fix:**
```c
if (txq->tx_pending_pkts == NULL) {
	/* allocation code */
} else {
	uint32_t i;
	for (i = 0; i <= txq->tx_pending_mask; i++)
		txq->tx_pending_pkts[i] = NULL;
}
```

---

## Warnings

### 1. Inconsistent loop counter types
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Lines:** 249, 408

At line 249, the implied type of `i` would be `uint16_t` (matching `tx_pending_mask`), but at line 408 it's declared as `uint32_t`. For consistency and to avoid potential narrowing issues with the mask comparison, use `uint32_t` in both places:

```c
/* Line 249 context */
uint32_t i;
for (i = 0; i <= txq->tx_pending_mask; i++)
	txq->tx_pending_pkts[i] = NULL;
```

### 2. Hardcoded socket ID breaks NUMA affinity
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Line:** 241

The allocation uses `SOCKET_ID_ANY` instead of the queue's socket ID. This defeats NUMA-local allocation for the shadow ring, which is accessed on every Tx completion. 

**Context:** The `ci_tx_queue` structure likely has a `socket_id` field (common pattern in DPDK PMDs). Use it:

```c
txq->tx_pending_pkts = rte_zmalloc_socket("idpf_tx_pending",
	sizeof(struct rte_mbuf *) * pending_sz,
	RTE_CACHE_LINE_SIZE, txq->socket_id);
```

If `ci_tx_queue` does not have a `socket_id` field, this should be added as part of the fix, since queue structures should always track their NUMA node.

### 3. Missing validation of compl_tag match
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Line:** 859

The completion path computes `tag = q_head & txq->tx_pending_mask` assuming `q_head` is the compl_tag echoed by hardware. However, there is no validation that this value matches `txd->qw1.compl_tag` from the completion descriptor. If hardware echoes a different value, or if there is a mismatch due to a bug, the wrong mbuf will be freed.

**Suggested fix:** Extract the actual compl_tag from the completion descriptor and use that:

```c
case IDPF_TXD_COMPLT_RS: {
	uint16_t compl_tag = txd->qw1.compl_tag;
	uint16_t tag = compl_tag & txq->tx_pending_mask;
	struct rte_mbuf *pkt = txq->tx_pending_pkts[tag];

	if (pkt != NULL) {
		rte_pktmbuf_free(pkt);
		txq->tx_pending_pkts[tag] = NULL;
	}
	break;
}
```

Note: This assumes the `txd` variable (the volatile completion descriptor pointer) is still in scope at this case statement. If not, it needs to be extracted earlier.

### 4. Missing release notes
This patch fixes a critical data corruption bug and changes internal data structures. It should be documented in the release notes under "Fixed Issues" for the net/idpf PMD.

**Suggested addition to `doc/guides/rel_notes/release_26_11.rst`:**

```rst
* **net/idpf: Fixed Tx payload corruption in split queue mode.**

  Fixed a use-after-free bug in split-queue Tx completion that caused
  payload corruption under high throughput. The completion path now uses
  a shadow ring indexed by completion tags instead of walking sw_ring slots.
```

---

## Info

### 1. Consider defensive NULL-check in Tx path
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Line:** 1070

If the allocation of `tx_pending_pkts` fails during reset (as discussed in Error #1), the Tx path will dereference a NULL pointer here:

```c
txq->tx_pending_pkts[tag] = tx_pkts[nb_tx];
```

Once the reset function is changed to return an error, this is less critical. However, adding a defensive check could prevent a crash if the queue somehow ends up in an invalid state:

```c
if (unlikely(txq->tx_pending_pkts == NULL)) {
	/* Log error and drop packet */
	rte_pktmbuf_free(tx_pkts[nb_tx]);
	continue;
}
txq->tx_pending_pkts[tag] = tx_pkts[nb_tx];
```

This is optional but improves robustness.

### 2. Shadow ring sizing documentation
**File:** `drivers/net/intel/common/tx.h`
**Line:** 265

The comment says "sized next_pow2(nb_tx_desc)" but the actual allocation is `rte_align32pow2(txq->nb_tx_desc)`. These are equivalent, but using `rte_align32pow2` in the comment would match the code exactly:

```c
/* ... shadow ring indexed by a rolling compl_tag ...
 * sized rte_align32pow2(nb_tx_desc).
 */
```

---

## Correctness Analysis

**The core fix is correct:** The use-after-free described in the commit message is real. The old code freed mbufs by walking `sw_ring[]` slots between `first_id` and EOP, but those slots can be reused by new submissions before the RS completion arrives. The new approach of stamping a unique `compl_tag` on the EOP descriptor and using it to index a shadow ring is the right solution.

**Key insight:** The completion tag echoed by hardware on RS is used as an index into `tx_pending_pkts[]`, which holds the packet-head mbuf until completion. This decouples mbuf lifetime from descriptor ring slot reuse, eliminating the race.

---

## Style

All style issues appear acceptable. The code follows DPDK conventions for indentation, bracing, and variable naming. No forbidden tokens detected.