|WARNING| [v2] 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/170802

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch fixes a critical use-after-free bug in the IDPF split-queue Tx path that was causing payload corruption. The fix introduces a shadow ring (`tx_pending_pkts`) to track mbuf ownership through RS completion, decoupling it from the descriptor ring slot lifecycle.

## Findings

### Errors

**1. Missing NULL check on txq->tx_pending_pkts before use**

In `idpf_dp_splitq_xmit_pkts()`, the code accesses `txq->tx_pending_pkts[tag]` without first verifying that `txq->tx_pending_pkts` itself is non-NULL. While the allocation happens at queue setup, a failure on a secondary path or an uninitialized queue could lead to NULL pointer dereference.

```c
/* Line 1022-1025 in idpf_common_rxtx.c */
uint16_t tag = txq->tx_next_compl_tag;

if (unlikely(txq->tx_pending_pkts[tag] != NULL)) {
    /* ... */
}
```

**Suggested fix:** Add a NULL check before accessing the array:

```c
if (unlikely(txq->tx_pending_pkts != NULL &&
             txq->tx_pending_pkts[tag] != NULL)) {
```

**2. Potential double-free on queue setup error path**

In `cpfl_tx_queue_setup()` (cpfl_rxtx.c:648), the error path calls `idpf_qc_split_tx_pending_free(txq)` which internally frees all mbufs in the array and then frees the array itself. However, the array was just allocated with `rte_zmalloc_socket()` which zero-initializes it, so all pointers are NULL. This is safe but the pattern could break if the allocation order changes. The same applies to `idpf_tx_queue_setup()`.

This is actually correct as written (zero-init ensures no stale pointers), but worth noting for maintainability.

**3. Resource leak on idpf_qc_split_tx_pending_alloc() failure in cpfl_tx_queue_setup()**

In `cpfl_tx_queue_setup()` at line 628-630, if `idpf_qc_split_tx_pending_alloc()` fails, the code jumps to `err_pending_alloc` which calls `idpf_qc_split_tx_pending_free()` (correct), but does not free `txq->rs_last_id` or `txq->sw_ring` which were allocated earlier. The error path should jump to the appropriate cleanup label that frees these resources.

```c
ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
if (ret != 0)
    goto err_pending_alloc;  /* Should be err_rs_last_id_alloc or similar */
```

**Suggested fix:**

```c
ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
if (ret != 0)
    goto err_rs_last_id_alloc;  /* Reuse existing label to free rs_last_id and sw_ring */
```

Same issue in `idpf_tx_queue_setup()` at line 498-500.

### Warnings

**1. tx_pending_size assignment is redundant**

In `idpf_qc_split_tx_pending_alloc()`, the comment states "Sized nb_tx_desc" and the code allocates `txq->nb_tx_desc` entries, but then assigns `txq->tx_pending_size = txq->nb_tx_desc`. This assignment is storing the size that was already used for allocation. While not incorrect, if `txq->tx_pending_size` is always equal to `txq->nb_tx_desc`, it could be removed and replaced with direct use of `nb_tx_desc`.

However, keeping it provides explicit bounds for array access and documents the intent, so this is acceptable. Just worth considering for simplification.

**2. Unnecessary explicit NULL assignment in idpf_qc_split_tx_pending_free()**

The function already sets `txq->tx_pending_pkts = NULL` at the end, so the individual NULL assignments in the loop (line 275) are redundant for the common case where the array is about to be freed.

```c
for (i = 0; i < txq->tx_pending_size; i++) {
    if (txq->tx_pending_pkts[i] != NULL)
        rte_pktmbuf_free(txq->tx_pending_pkts[i]);
        /* No need: txq->tx_pending_pkts[i] = NULL; */
}
```

However, the function is also called from `idpf_qc_split_tx_descq_reset()` where the array persists after reset, so the NULL assignments there (line 240) are necessary and correct.

### Info

**1. Release notes coverage**

The patch fixes a critical correctness bug (payload corruption due to use-after-free). Verify that the fix is documented in the release notes under "Fixed Issues" for the target release.

**2. Error message clarity**

At line 877, the error message "invalid completion tag %u" is good, but could be more actionable:

```c
TX_LOG(ERR, "invalid completion tag %u (>= %u).", tag, txq->tx_pending_size);
```

This helps debugging by showing both the bad value and the limit.

**3. Shadow ring naming consistency**

The array is named `tx_pending_pkts` in the structure and code, but allocation error messages say "tx_pending shadow ring". Consider using "tx_pending_pkts" in the message for consistency:

```c
DRV_LOG(ERR, "Failed to alloc tx_pending_pkts array");
```

---

## Positive Observations

1. **Correctness:** The fix correctly addresses the root cause (mbuf lifetime decoupled from descriptor slot reuse) by using a shadow ring indexed by a rolling completion tag.

2. **Spec compliance:** The patch correctly applies the same `compl_tag` to all data descriptors of a multi-segment packet per the IDPF spec.

3. **Bounds checking:** The completion handler validates the tag against `tx_pending_size` before array access (line 879).

4. **Error propagation:** Allocation failures return `-ENOMEM` and are propagated correctly in the setup paths.

5. **Cleanup:** All setup error paths and release functions now correctly free the shadow ring.

6. **Atomicity:** Code and documentation are updated together.

---

## Summary by Severity

**Errors:** 3 (NULL check missing, resource leaks on error paths)  
**Warnings:** 2 (redundant field, code pattern observation)  
**Info:** 3 (release notes, error message clarity, naming)

The critical correctness bug (use-after-free causing corruption) is fixed correctly. The errors listed above are secondary issues in error handling paths that should be addressed before merging.