|SUCCESS| [v4] net/intel: fix idpf Tx payload corruption in split queue

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**1. Missing `rte_free()` on split queue allocation failure path**

In `cpfl_tx_queue_setup()` at line 621, when `idpf_qc_split_tx_pending_alloc()` fails, the code jumps to `err_sw_ring_alloc`, which calls `rte_memzone_free(mz)` but does not free `txq->tx_pending_pkts` that may have been allocated. The same pattern exists in `idpf_tx_queue_setup()` at line 493.

However, examining `idpf_qc_split_tx_pending_alloc()` shows it only allocates on success and leaves `tx_pending_pkts` NULL on failure, so there is no leak here. The error path is correct.

**Actually this is correct** - on reviewing the allocation function, it returns -ENOMEM without allocating anything if `rte_zmalloc_socket()` fails, so `tx_pending_pkts` remains NULL and there's nothing to free. No issue.

**2. Potential buffer overrun in split queue tag validation**

In `idpf_split_tx_free()` at line 862:
```c
uint16_t tag = q_head;

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

The code validates `tag` but continues execution after logging the error. If `tag >= txq->nb_tx_desc`, the `else if` is skipped, but the function does not return early, and later code paths may use the invalid `tag` value. This could lead to out-of-bounds access.

Wait, re-reading the code: the structure is `if (condition) { log } else if (safe condition) { use }`. If the first condition is true (invalid tag), the else-if is not executed, so `tx_pending_pkts[tag]` is not accessed. The function then returns at line 870 after the closing brace. This is actually safe - the `else if` protects the array access.

**Actually this is correct** - the else-if prevents access when tag is invalid. No issue.

**3. Race condition: completion queue freed while in use**

In `cpfl_tx_queue_release()` and `idpf_qc_tx_queue_release()`, the code frees the completion queue (`rte_memzone_free(q->complq->mz)`) without first ensuring that no completion processing is in flight. The data plane could be executing `idpf_split_tx_free()` on another core, accessing `cq->compl_ring`, when the memzone is freed.

However, `tx_queue_release()` is a control-path operation called when the queue is stopped and the device is not running. DPDK's ethdev API requires the application to stop the queue before releasing it, so there should be no concurrent datapath access. This is a control-path vs control-path concern, and ethdev requires serialization of control operations.

**Not a bug** - ethdev API contract requires the queue to be stopped before release. No concurrent datapath access is possible if the API is used correctly.

## WARNINGS

**1. Completion queue not drained before descriptor queue stop in `cpfl_tx_queue_stop()`**

In `cpfl_tx_queue_stop()` at line 1368:
```c
if (vport->txq_model == VIRTCHNL2_QUEUE_MODEL_SINGLE) {
    ci_txq_release_all_mbufs(txq, txq->use_vec_entry);
    idpf_qc_single_tx_queue_reset(txq);
} else {
    if (cpfl_txq->hairpin_info.hairpin_q) {
```

For split queues, the code calls `idpf_qc_split_tx_descq_reset()` (visible in context, line 1374) without first draining the completion queue. If the hardware has posted completions but the driver has not yet processed them, `idpf_qc_split_tx_descq_reset()` will call `idpf_qc_split_tx_release_mbufs()` (per the patch), freeing mbufs based on the shadow ring state. But if completions are pending, the shadow ring may still reference mbufs for packets the hardware has not yet confirmed, leading to premature free.

Wait, reviewing the reset function: `idpf_qc_split_tx_descq_reset()` calls `idpf_qc_split_tx_release_mbufs()` which drains the entire shadow ring (all `nb_tx_desc` slots). So it frees every mbuf, regardless of completion state. If the queue is stopped, the hardware will not DMA to those buffers anymore, so freeing them is safe. This is the stop path, not the runtime path.

**Actually acceptable** - queue stop implies hardware has been told to stop, so draining all pending mbufs is the correct cleanup. No issue.

**2. `use_vec_entry` logic may not match actual Tx function selection**

In `cpfl_set_tx_function()` at line 1550:
```c
txq->use_vec_entry = (ad->tx_func_type != IDPF_TX_SINGLEQ &&
        ad->tx_func_type != IDPF_TX_DEFAULT);
```

And in `idpf_set_tx_function()` at line 894:
```c
txq->use_vec_entry = (ad->tx_func_type != IDPF_TX_SINGLEQ &&
        ad->tx_func_type != IDPF_TX_DEFAULT);
```

The comment says "Set use_vec_entry for single queue mode - only IDPF_TX_SINGLEQ uses regular entries", but the code sets it for ALL queue models based on `tx_func_type`. For split queues, `use_vec_entry` now controls whether `idpf_qc_split_tx_release_mbufs()` drains via `ci_txq_release_all_mbufs()` (vector) or loops over `tx_pending_pkts` (scalar).

But `tx_func_type` can be `IDPF_TX_DEFAULT` (scalar split queue transmit), yet the code would set `use_vec_entry = false`. Then `idpf_qc_split_tx_release_mbufs()` would drain the scalar shadow ring, which is correct. And if `tx_func_type` is a vector type, `use_vec_entry = true`, so it drains via `ci_txq_release_all_mbufs()`, which is correct.

**Actually this is correct** - the logic matches the split queue allocation pattern where scalar uses `tx_pending_pkts` and vector uses the sw_ring_vec. No issue.

## INFO

None.

---

After three verification passes:
- Pass 1 (correctness bugs): No bugs identified. All traced paths are safe.
- Pass 2 (style/process false positives): All items removed as correct.
- Pass 3 (evidence verification): All findings removed as correct on inspection.

The patch introduces a shadow ring indexed by completion tags to fix split-queue mbuf lifetime management. The core correctness logic is sound:
- Tag allocation checks for occupancy before use
- Completion processing only accesses valid tag indices  
- Cleanup paths drain the entire shadow ring on stop/release
- Error path cleanup is correct (allocation failure leaves pointers NULL)

Review-Result: CLEAN