|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. Missing error-path cleanup of tx_pending_pkts contents**

In `idpf_qc_split_tx_pending_alloc()`, if the `rte_zmalloc_socket()` fails (returns NULL), the function returns `-ENOMEM`. However, if setup continues and later fails, the `idpf_qc_split_tx_pending_free()` is called, which will try to iterate `txq->nb_tx_desc` entries when `txq->tx_pending_pkts` is NULL. The NULL check in `idpf_qc_split_tx_pending_free()` prevents a crash, but the design is fragile.

More importantly, in `idpf_qc_split_tx_descq_reset()`:

```c
if (txq->tx_pending_pkts != NULL) {
    for (i = 0; i < txq->nb_tx_desc; i++) {
        if (txq->tx_pending_pkts[i] != NULL) {
            rte_pktmbuf_free(txq->tx_pending_pkts[i]);
            txq->tx_pending_pkts[i] = NULL;
        }
    }
}
```

The variable `i` is declared at the top of the function but is only used inside this conditional block. If `txq->tx_pending_pkts` is NULL, the variable `i` is unused. This is a minor style issue but should be fixed by declaring `i` inside the block:

```c
if (txq->tx_pending_pkts != NULL) {
    for (uint32_t i = 0; i < txq->nb_tx_desc; i++) {
```

**2. Resource leak on error path in tx_queue_setup**

In `idpf_tx_queue_setup()` (idpf_rxtx.c) and `cpfl_tx_queue_setup()` (cpfl_rxtx.c):

```c
err_complq_setup:
	idpf_qc_split_tx_pending_free(txq);
err_pending_alloc:
	rte_free(txq->rs_last_id);
```

The `err_complq_setup` label calls `idpf_qc_split_tx_pending_free(txq)`, which frees any mbufs in the pending array and then frees the array itself. However, if `idpf_qc_split_tx_complq_setup()` partially initializes the completion queue before failing, there may be resources in `txq->complq` that need cleanup. The code does:

```c
if (!is_splitq) {
    // ...
} else {
    txq->desc_ring = mz->addr;
    ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
    if (ret != 0)
        goto err_pending_alloc;
    idpf_qc_split_tx_descq_reset(txq);

    /* Setup tx completion queue if split model */
    ret = idpf_qc_split_tx_complq_setup(dev, txq, queue_idx,
                                        socket_id, tx_conf);
    if (ret != 0)
        goto err_complq_setup;
}
```

If `idpf_qc_split_tx_complq_setup()` allocates `txq->complq` but then fails partway through, the error path does not call any cleanup for `txq->complq`. Looking at the release path in `idpf_qc_tx_queue_release()`:

```c
if (q->complq != NULL) {
    rte_free(q->complq);
}
```

This suggests that `txq->complq` should be freed on the `err_complq_setup` path. The patch adds the pending cleanup but does not add complq cleanup. This is a potential memory leak if `idpf_qc_split_tx_complq_setup()` allocates `txq->complq` and then fails.

**Suggested fix:**

```c
err_complq_setup:
	if (txq->complq != NULL)
		rte_free(txq->complq);
	idpf_qc_split_tx_pending_free(txq);
err_pending_alloc:
```

Apply the same fix to both `idpf_tx_queue_setup()` and `cpfl_tx_queue_setup()`.

---

## WARNINGS

**1. Inconsistent error message capitalization**

In `idpf_split_tx_free()`:

```c
TX_LOG(ERR, "invalid completion tag %u.", tag);
```

and

```c
TX_LOG(ERR, "unknown completion type.");
```

DPDK style prefers error messages to start with a capital letter. The first message starts lowercase, the second starts lowercase (but "unknown" is the first word). For consistency, both should start with a capital:

```c
TX_LOG(ERR, "Invalid completion tag %u.", tag);
```

**2. Comment style: missing space after /*

In `drivers/net/intel/common/tx.h`:

```c
/* Split-Tx completion tracking: shadow ring indexed by a
 * rolling compl_tag decouples RS-completion mbuf lifetime
 * from sw_ring[] slot reuse. Sized nb_tx_desc.
 */
```

This is a multi-line comment and follows the correct style. No issue here (flagging withdrawn).

**3. Variable declaration style**

In `idpf_dp_splitq_xmit_pkts()`:

```c
uint16_t tag = txq->tx_next_compl_tag;

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

The variable `tag` is declared in the middle of the function body, not at the top of the block. This is acceptable C99 style per the guidelines ("Variables may be declared either at the start of the block, or at point of first use"). However, placing it just before the conditional is clearer. This is stylistically acceptable, so no change needed (flagging withdrawn).

**4. Missing release notes update**

The patch fixes a correctness bug (payload corruption due to use-after-free) and adds a new error path (`-ENOMEM` from queue setup). This is a significant change that should be documented in the release notes. The commit message mentions the fix is for a bug introduced in commit `96cc9b6ea60c`, but does not update `doc/guides/rel_notes/`.

**Suggested action:** Add a release notes entry under "Fixed Issues" describing the Tx payload corruption fix and the new allocation failure path.

---

## INFO

**1. Comment could be clearer about tag size**

In `idpf_dp_splitq_xmit_pkts()`:

```c
/* RE reclaims descriptors before RS, so tag occupancy is the
 * only bound on packets awaiting RS completion.
 */
```

This comment explains why the pending array must be checked, but it could be clearer that the tag space is sized to `nb_tx_desc` (same as the descriptor ring) to prevent overflow. Consider:

```c
/* RE reclaims descriptors before RS, so tag occupancy is the
 * only bound on packets awaiting RS completion. Shadow ring is
 * sized nb_tx_desc to match the descriptor ring and prevent
 * tag wraparound collisions.
 */
```

**2. Spec reference could be more precise**

The comment:

```c
/* Spec: COMPLETION_TAG must be identical on all data descs of a packet. */
```

is helpful, but it would be even better if it cited the specific section of the IDPF specification. This helps future maintainers verify the requirement.

---

## SUMMARY

The patch correctly addresses the original use-after-free bug by introducing a shadow ring indexed by completion tag. The core logic is sound: decouple mbuf lifetime from descriptor slot reuse, track pending packets in a separate array, and clean them up on RS completion.

However, there are two resource-leak risks on error paths:
1. `txq->complq` is not freed if `idpf_qc_split_tx_complq_setup()` fails after allocating it
2. Variable `i` in `idpf_qc_split_tx_descq_reset()` should be declared inside the conditional block (minor style)

The missing release notes entry is also a process requirement for a fix of this significance.

---

Review-Result: ERROR