|WARNING| [v3] net/intel: fix idpf Tx payload corruption in split queue

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-22

# DPDK Patch Review

## ERRORS

### 1. Error path resource leak - missing free on error
**File:** `drivers/net/intel/cpfl/cpfl_rxtx.c`
**Location:** Lines 624-642

The `cpfl_tx_queue_setup()` error path at `err_complq_setup:` calls `idpf_qc_split_tx_pending_free(txq)` but this happens **after** the label. If `idpf_qc_split_tx_complq_setup()` fails, the code jumps to `err_complq_setup:` and executes the free. However, if `idpf_qc_split_tx_pending_alloc()` itself fails, execution jumps to `err_pending_alloc:` which then falls through to `err_rs_last_id_alloc:` **without** freeing `txq->rs_last_id` or `txq->sw_ring` that were allocated earlier (lines 597-614).

The same error exists in `idpf_rxtx.c` at the identical location (lines 495-522).

**Suggested fix:**
```c
err_pending_alloc:
	rte_free(txq->rs_last_id);
err_rs_last_id_alloc:
	rte_free(txq->sw_ring);
```
These two labels should exist but are missing from the patch context shown. Verify they exist and are reachable from the new `err_pending_alloc:` label.

### 2. Missing NULL check before access - dereference risk
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Location:** Line 873

```c
if (unlikely(tag >= txq->nb_tx_desc)) {
	TX_LOG(ERR, "invalid completion tag %u.", tag);
} else if (txq->tx_pending_pkts[tag] != NULL) {
```

The `else if` branch accesses `txq->tx_pending_pkts[tag]` without verifying that `txq->tx_pending_pkts` itself is non-NULL. While `idpf_split_tx_free()` is only called after successful queue setup where allocation succeeded, defensive code should check the pointer, especially given the `TX_LOG(ERR)` suggests this path handles unexpected/corrupt hardware behavior.

**Suggested fix:**
```c
if (unlikely(tag >= txq->nb_tx_desc)) {
	TX_LOG(ERR, "invalid completion tag %u.", tag);
} else if (txq->tx_pending_pkts != NULL && 
           txq->tx_pending_pkts[tag] != NULL) {
	rte_pktmbuf_free(txq->tx_pending_pkts[tag]);
	txq->tx_pending_pkts[tag] = NULL;
}
```

### 3. Inconsistent error handling - silent failure after log
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Location:** Line 873

The `if (unlikely(tag >= txq->nb_tx_desc))` branch logs an error but does **not** return early. Execution falls through to the `else if` which will not execute (since the condition is false), but then continues to `break;` at line 883. This means an out-of-bounds tag is treated as a successful completion, allowing the caller to continue processing completions with potentially corrupted state.

**Suggested fix:**
```c
if (unlikely(tag >= txq->nb_tx_desc)) {
	TX_LOG(ERR, "invalid completion tag %u.", tag);
	return;  /* or set error flag and break */
}
if (txq->tx_pending_pkts != NULL && txq->tx_pending_pkts[tag] != NULL) {
	rte_pktmbuf_free(txq->tx_pending_pkts[tag]);
	txq->tx_pending_pkts[tag] = NULL;
}
break;
```

---

## WARNINGS

### 1. Missing release notes update
The patch fixes a critical data corruption bug but does not add or modify any file under `doc/guides/rel_notes/`. A fix of this severity (corrupted payload under sustained traffic) should be documented in the release notes with a brief description and the Fixes tag.

**Suggested fix:** Add an entry to `doc/guides/rel_notes/release_NN_MM.rst` (where NN_MM is the target release, e.g., 26.11):
```rst
* **Fixed Intel IDPF split-queue Tx payload corruption.**

  Fixed a bug in the IDPF driver where sustained high-throughput split-queue
  Tx could corrupt packet payloads. The completion path freed mbufs prematurely
  before DMA completed, causing buffers to be recycled and overwritten mid-transfer.
```

### 2. Potential integer overflow in allocation size
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Location:** Line 252

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

If `txq->nb_tx_desc` is very large (e.g., 32768), the multiplication `sizeof(struct rte_mbuf *) * txq->nb_tx_desc` could overflow a 32-bit intermediate result on 32-bit architectures before widening to `size_t` for the allocation. While `nb_tx_desc` is bounded by device limits (typically 8192), future hardware or misconfigurations could exceed safe values.

**Suggested fix:**
```c
txq->tx_pending_pkts = rte_zmalloc_socket("idpf_tx_pending",
		(size_t)txq->nb_tx_desc * sizeof(struct rte_mbuf *),
		RTE_CACHE_LINE_SIZE, socket_id);
```

---

## INFO

### 1. Comment clarity - "sole source of truth"
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Location:** Line 869-871

The comment states "Shadow ring indexed by the software-defined compl_tag is the sole source of truth for RS-time mbuf ownership". While accurate, it would be clearer to note that this decoupling is necessary **because** `sw_ring[]` slots are reused by RE before RS completes, which is the root cause this patch fixes.

**Suggested addition:**
```c
/* Shadow ring indexed by the software-defined compl_tag is
 * the sole source of truth for RS-time mbuf ownership. This
 * decoupling is necessary because RE reclaims and reuses
 * sw_ring[] slots before RS lands, so sw_ring[].mbuf cannot
 * track in-flight ownership.
 */
```

### 2. Variable name clarity - `tag` in free path
**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`
**Location:** Line 875

```c
uint16_t tag = q_head;
```

The name `tag` is clear in context, but the relationship `tag = q_head` (where `q_head` is the completion descriptor index) could be documented. A brief comment would help future readers understand why the descriptor index doubles as the tag.

**Suggested addition:**
```c
/* Completion tag equals the descriptor index at RS time. */
uint16_t tag = q_head;
```

---

## SUMMARY

The patch correctly solves a serious data corruption bug by decoupling RS-completion mbuf lifetime from descriptor slot reuse via a shadow ring. The core logic is sound. However, three **Errors** must be fixed:

1. Missing error-path cleanup for `rs_last_id` and `sw_ring` when pending allocation fails
2. Missing NULL check on `txq->tx_pending_pkts` before array access in the completion path
3. Silent continuation after logging an out-of-bounds tag error

Two **Warnings** should be addressed:
1. Add a release notes entry for this critical bug fix
2. Cast `nb_tx_desc` to `size_t` before multiplication to prevent potential overflow

Review-Result: ERROR