|WARNING| [4/4] pdump: undo partial enable on failure

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/4: ethdev: reject hairpin queue in add first Rx callback

### Errors

**Use-after-free risk in error path**

Line 6181 in `rte_eth_add_first_rx_callback()`:
```c
struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);

if (cb == NULL) {
```

The patch adds a hairpin check before the `rte_zmalloc()` call.
However, the existing code after `rte_zmalloc()` has multiple error paths
that assume `cb` is non-NULL when it may be NULL on allocation failure.
While not introduced by this patch (pre-existing bug), the patch adds a new early return
that could mask whether the subsequent allocation succeeded.

**Actually, re-checking**: The new hairpin check returns before `rte_zmalloc()`,
so this is not introduced by the patch. Correction withdrawn.

---

## Patch 2/4: pdump: fix error code when adding callback fails

No issues found.

---

## Patch 3/4: pdump: skip hairpin queues

### Errors

**Error path resource leak**

Lines 283-286 and 345-348 in `pdump_register_rx_callbacks()` and `pdump_register_tx_callbacks()`:
```c
if (operation == ENABLE) {
    if (cbs->cb) {
        PDUMP_LOG_LINE(ERR, ...);
        return -EEXIST;
    }
```

When the hairpin check returns `-EINVAL` (lines 273 and 335), or when `cbs->cb` already exists
and returns `-EEXIST`, the function exits without releasing the `filter` parameter.
The `filter` is an `rte_bpf *` allocated by the caller (`set_pdump_rxtx_cbs()`)
via `rte_bpf_elf_load()`, and ownership is transferred to these register functions on ENABLE.
The early returns leak the BPF filter.

Note: This is a pre-existing bug exposed by the new error paths the patch adds,
but since the patch introduces new early returns without cleanup, it extends the leak surface.

---

## Patch 4/4: pdump: undo partial enable on failure

### Errors

**Double-free on `pdump_unwind_rx_callbacks()` -> `pdump_unwind_tx_callbacks()` sequence**

Lines 505-513 in `set_pdump_rxtx_cbs()`:
```c
unwind_rx:
    if (operation == ENABLE && (flags & RTE_PDUMP_FLAG_RX)) {
        end_q = (queue == RTE_PDUMP_ALL_QUEUES) ? nb_rx_q : queue + 1;
        pdump_unwind_rx_callbacks(port,
            (queue == RTE_PDUMP_ALL_QUEUES) ? 0 : queue, end_q);
    }
free_filter:
    if (filter != NULL)
        rte_bpf_destroy(filter);
```

When Tx callback installation fails (line 535 `goto unwind_rx`), the code jumps to `unwind_rx` label,
which calls `pdump_unwind_rx_callbacks()` to remove the Rx callbacks installed earlier.
Then it falls through to `free_filter`, which calls `rte_bpf_destroy(filter)`.

However, `filter` is shared between Rx and Tx. In the ENABLE path:
- Rx callbacks store `filter` in `cbs->filter` (line 321: `cbs->filter = filter;`)
- Tx callbacks also store the same `filter` pointer (line 393: `cbs->filter = filter;`)
- When `pdump_unwind_rx_callbacks()` removes the Rx callbacks,
  it does NOT call `rte_bpf_destroy()` on `cbs->filter` for each queue
  (the `pdump_unwind_rx_callbacks()` function at line 256 only nulls `cbs->cb`,
  not `cbs->filter`)
- But the shared `filter` is then destroyed once at line 511

This is actually correct: the filter is allocated once in `set_pdump_rxtx_cbs()` (line 461-470),
shared across all callbacks, and destroyed once on the error path.

**Correction withdrawn** - the filter is single-allocated, multi-referenced, single-freed. This is correct.

---

**Missing `filter` cleanup in existing error paths**

Pre-existing bug now visible: lines 492, 498, 503, 507, 512, 518 in `set_pdump_rxtx_cbs()`:
```c
goto free_filter;
```

The patch adds `goto free_filter` on error paths where previously the code did `return ret`.
However, several of these paths occur before the Rx callback registration (lines 522-526).
If the error occurs before any callbacks are registered,
the `unwind_rx` label (lines 505-513) is skipped by jumping directly to `free_filter`,
which is correct.

But if Rx callbacks were registered successfully and Tx callback registration fails,
the `goto unwind_rx` correctly unwinds Rx, then falls through to `free_filter`.

**Correction**: The flow is correct. `unwind_rx` calls `pdump_unwind_rx_callbacks()`,
then falls through to `free_filter` which calls `rte_bpf_destroy(filter)`. No double-free.

---

**Actually, checking the callback unwind functions more carefully**:

Lines 256-268 `pdump_unwind_rx_callbacks()`:
```c
static void
pdump_unwind_rx_callbacks(uint16_t port, uint16_t first, uint16_t last)
{
    uint16_t qid;

    for (qid = first; qid < last; qid++) {
        struct pdump_rxtx_cbs *cbs = &rx_cbs[port][qid];

        if (cbs->cb == NULL)
            continue;
        rte_eth_remove_rx_callback(port, qid, cbs->cb);
        pdump_cb_wait(cbs);
        cbs->cb = NULL;
    }
}
```

The unwind function removes the callback but does NOT clear `cbs->filter`.
The filter pointer remains in the `pdump_rxtx_cbs` structure.
However, the filter is then destroyed at the `free_filter` label.

If another enable request comes in after this partial failure,
it will see `cbs->cb == NULL` (unwound) but `cbs->filter` may still be a dangling pointer
to the freed filter. The next enable will compare `cbs->cb` (now NULL),
see it's NULL, and install a new callback, but the old filter pointer is stale.

**Wait**, checking the enable path (line 277-328):
```c
if (operation == ENABLE) {
    if (cbs->cb) {
        PDUMP_LOG_LINE(ERR, ...);
        pdump_unwind_rx_callbacks(port, first, qid);
        return -EEXIST;
    }
    cbs->use_count = 0;
    cbs->ver = ver;
    cbs->snaplen = snaplen;
    cbs->ring = ring;
    cbs->mp = mp;
    cbs->filter = filter;
    cbs->cb = rte_eth_add_first_rx_callback(port, qid, pdump_rx, cbs);
    ...
}
```

When enabling, the code unconditionally assigns `cbs->filter = filter` (line 321).
It does not check if `cbs->filter` is stale from a previous failed enable.
So if unwind cleared `cbs->cb` but left `cbs->filter` dangling,
the next enable overwrites it, which is fine (no leak, no use-after-free).

**Correction withdrawn** - the enable path overwrites all fields including `cbs->filter`,
so stale pointers are not used.

---

**Missing cleanup of `cbs->filter` on DISABLE**

Lines 339-350 (similar in Tx at 416-427):
```c
} else if (operation == DISABLE) {
    if (cbs->cb == NULL) {
        PDUMP_LOG_LINE(ERR, ...);
        return -EINVAL;
    }
    ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
    if (ret < 0) {
        PDUMP_LOG_LINE(ERR, ...);
        return ret;
    }
    pdump_cb_wait(cbs);
    cbs->cb = NULL;
}
```

On DISABLE, the code removes the callback and sets `cbs->cb = NULL`,
but does NOT set `cbs->filter = NULL` or call `rte_bpf_destroy(cbs->filter)`.

The filter was allocated in `set_pdump_rxtx_cbs()` (line 461-470, only on ENABLE)
and is shared across all queues that were enabled in one request.
On DISABLE, if the filter is not freed, it leaks.

However, checking the DISABLE path in `set_pdump_rxtx_cbs()`:
There is no filter allocation on DISABLE (lines 461-470 are inside `if (operation == ENABLE)`).
And there is no filter destruction on DISABLE either.

**This is a pre-existing resource leak**: the BPF filter allocated on ENABLE
is never freed on DISABLE. Since the patch does not touch DISABLE paths,
this is pre-existing, but worth noting.

**Report as Info (pre-existing)**.

---

## Summary

### Patch 1/4
Clean.

### Patch 2/4
Clean.

### Patch 3/4
**Error**: Hairpin check early returns leak the `filter` parameter (lines 273, 335).
The filter is allocated by the caller and not freed on these new error paths.

### Patch 4/4
Clean. The unwind logic correctly removes callbacks and frees the filter on the error path.

---

## Info (pre-existing issues)

**BPF filter not freed on DISABLE** (pre-existing, not introduced by this series):
In `pdump_register_rx_callbacks()` and `pdump_register_tx_callbacks()`,
when `operation == DISABLE`, the code removes the callback but does not free `cbs->filter`.
The filter allocated during ENABLE is leaked. This affects all DISABLE operations,
not just those in this patch.

---

Review-Result: ERROR