|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 correctness bug** (use-after-free in Tx completion) by introducing a shadow ring to decouple descriptor-slot reuse from mbuf lifetime tracking. The fix is sound, but there are several process and style issues to address.

---

## Errors (must fix)

### 1. `rte_zmalloc_socket()` with `SOCKET_ID_ANY`

**Location:** `idpf_common_rxtx.c:240`

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

**Problem:** Queue-related buffers must be NUMA-local to the queue's socket. Using `SOCKET_ID_ANY` defeats NUMA affinity and can cause cross-socket memory access in the fast path.

**Fix:** Pass the queue's socket ID (available in the queue context or from the device):

```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 `txq->socket_id` is not available, trace back to where `txq` is allocated--the socket ID used there should be passed here.

---

### 2. Resource leak on allocation failure

**Location:** `idpf_common_rxtx.c:240-243`

```c
if (txq->tx_pending_pkts == NULL) {
    DRV_LOG(ERR, "Failed to alloc idpf tx_pending shadow ring");
    return;
}
```

**Problem:** The function `idpf_qc_split_tx_descq_reset()` returns `void`, so the allocation failure is logged but the queue remains in an inconsistent state. The queue should not be used after this failure, but the caller has no way to know allocation failed.

**Impact:** If allocation fails, subsequent Tx operations will dereference `NULL` (`txq->tx_pending_pkts[tag]`), causing a crash.

**Fix:** Either:

1. Change the function signature to return `int` and propagate the error to the caller (preferred), or
2. Mark the queue as unusable (e.g., set a flag or `txq->nb_tx_desc = 0`) so later operations fail gracefully.

The preferred fix:

```c
/* In tx.h: change signature */
int idpf_qc_split_tx_descq_reset(struct ci_tx_queue *txq);

/* In idpf_common_rxtx.c */
if (txq->tx_pending_pkts == NULL) {
    DRV_LOG(ERR, "Failed to alloc idpf tx_pending shadow ring");
    return -ENOMEM;
}
/* ... rest of function ... */
return 0;

/* Update all callers to check the return value */
```

---

### 3. `for` loop iterator declared outside C99 block scope

**Location:** `idpf_common_rxtx.c:230, 248, 408`

```c
uint16_t i;
/* ... */
for (i = 0; i < sw_nb_desc; i++)
    txq->sw_ring[i].mbuf = NULL;
```

**Problem:** The iterator `i` is declared at the top of the function, far from its use. DPDK coding style allows (and prefers for clarity) declaring the loop iterator in the `for` statement when the variable is not used outside the loop.

**Fix:**

```c
for (uint16_t i = 0; i < sw_nb_desc; i++)
    txq->sw_ring[i].mbuf = NULL;
```

Apply the same fix to the other two loops (lines 248 and 408).

---

### 4. Boolean expression: should use explicit comparison

**Location:** `idpf_common_rxtx.c:236, 410`

```c
if (txq->tx_pending_pkts == NULL) {
```

and

```c
if (q->tx_pending_pkts != NULL) {
```

**Problem:** DPDK style requires explicit comparison against `NULL` for pointers. However, both of these **are already explicit**--this is correct. (This note is to confirm: **do NOT flag this**.)

Similarly, line 862:

```c
if (pkt != NULL) {
```

is also correct. **No change needed.**

---

## Warnings (should fix)

### 1. Missing release notes update

**Problem:** This patch fixes a critical data-corruption bug (`Fixes:` tag and `Cc: stable at dpdk.org` present) but does not update the release notes to document the fix.

**Required:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or the appropriate current release file) under "**Fixed Issues**" or "**Drivers**" section:

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

  Fixed a use-after-free bug in split-queue Tx completion that caused
  in-flight mbuf buffers to be recycled prematurely, corrupting transmitted
  packet payloads under sustained high-throughput workloads.
```

---

### 2. Loop variable scope (style preference)

**Location:** `idpf_common_rxtx.c:248, 408`

The loops iterating over `tx_pending_pkts[]` declare `i` at the top of their enclosing block. Per the guideline update, this is acceptable, but declaring at point of use (C99 style) is clearer:

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

This matches the existing preference in DPDK for short, isolated loop counters.

---

## Info (consider)

### 1. Comment clarity: "Rolling compl_tag"

**Location:** `idpf_common_rxtx.c:1067`

```c
/* Stamp EOP with the rolling compl_tag; RS uses it to look up
 * the packet-head mbuf in the shadow ring.
 */
```

The comment is clear and helpful. No change needed, but you could optionally add a note in the header struct comment that `tx_next_compl_tag` wraps modulo `tx_pending_mask + 1` for completeness.

---

### 2. Potential for `tx_pending_mask` to be wrong size on non-power-of-2 `nb_tx_desc`

**Location:** `idpf_common_rxtx.c:238`

```c
uint32_t pending_sz = rte_align32pow2(txq->nb_tx_desc);
```

If `nb_tx_desc` is already a power of 2, this is fine. If it's not, `rte_align32pow2()` rounds **up** to the next power of 2, which is correct (you need at least as many shadow slots as descriptors). But if `nb_tx_desc` is very large (e.g., close to 2^16), doubling it could waste memory.

**Recommendation:** Verify that `nb_tx_desc` is always a power of 2 (most DPDK queue sizes are), or add a comment explaining why the round-up is safe. If `nb_tx_desc` can be arbitrary, consider enforcing a power-of-2 requirement at queue setup time.

---

## Additional Notes

### Correctness of the fix

The patch correctly addresses the use-after-free described in the commit message:

- **Before:** `sw_ring[]` slots were reused by the RE (reorder engine) before RS completion, so the completion path was freeing mbufs still in flight.
- **After:** A shadow ring indexed by a software-defined `compl_tag` (stamped on the EOP descriptor and echoed by HW) decouples mbuf lifetime from descriptor slot reuse.

The shadow ring is sized to `next_pow2(nb_tx_desc)` to ensure there are enough slots for all in-flight packets. The `compl_tag` is a rolling counter masked to the ring size, so it wraps correctly.

### Alignment with DPDK patterns

The shadow-ring approach is similar to patterns used in other DPDK PMDs (e.g., `i40e`, `ice`) for split-queue or out-of-order completion scenarios. The fix is architecturally sound.

---

## Checklist Summary

- [x] Correctness bug identified and fixed
- [ ] **Error:** `SOCKET_ID_ANY` should be replaced with queue's socket ID
- [ ] **Error:** Allocation failure must propagate error or mark queue unusable
- [ ] **Error:** Loop iterators should use C99 declaration-in-for style
- [ ] **Warning:** Release notes must document the fix
- [x] No use-after-free in the modified code
- [x] No resource leaks (shadow ring freed on queue release)
- [x] No race conditions introduced
- [x] Explicit NULL comparisons used correctly
- [x] Comments are clear and helpful

---

## Final Recommendation

**Do not merge** until the two **Errors** (SOCKET_ID_ANY, allocation failure handling) are fixed and release notes are added. The loop iterator style changes are lower priority but should be addressed for consistency.