|WARNING| [v3] pdump: fix teardown race with opt-in MP request mode

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch adds a new EAL multiprocess request mode and hardened pdump teardown logic to prevent race conditions during disable. The changes introduce generation tracking, inflight callback counting, and a new `rte_mp_request_sync_ex()` API with an opt-in flag to ignore peers without registered actions.

---

## Errors

### 1. `pdump_wait_inflight()` timeout calculation vulnerable to timer wrap

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_wait_inflight()`

The timeout calculation `end_tsc = rte_get_timer_cycles() + MP_TIMEOUT_S * rte_get_timer_hz()` can overflow on platforms where `rte_get_timer_cycles()` is close to `UINT64_MAX`. The comparison `rte_get_timer_cycles() > end_tsc` will then incorrectly return timeout immediately.

**Fix:** Use a deadline-based approach with wraparound-safe comparison, or validate that `end_tsc > start_tsc` before entering the loop:

```c
static int
pdump_wait_inflight(struct pdump_shared_ctl *ctl)
{
	uint64_t start_tsc = rte_get_timer_cycles();
	uint64_t timeout_tsc = MP_TIMEOUT_S * rte_get_timer_hz();
	uint64_t elapsed;

	while (rte_atomic_load_explicit(&ctl->inflight, rte_memory_order_acquire) != 0) {
		elapsed = rte_get_timer_cycles() - start_tsc;
		if (elapsed > timeout_tsc)
			return -ETIMEDOUT;
		rte_pause();
	}

	return 0;
}
```

---

### 2. Resource leak: `filter` not released on RX callback enable failure

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_register_rx_callbacks()`

When `rte_eth_add_first_rx_callback()` fails after successfully reconciling a stale callback:

```c
cbs->filter = filter;
cbs->generation = generation;

cbs->cb = rte_eth_add_first_rx_callback(port, qid,
					pdump_rx, cbs);
if (cbs->cb == NULL) {
	PDUMP_LOG_LINE(ERR, "failed to add rx callback, errno=%d",
			rte_errno);
	return rte_errno;  /* LEAK: filter is not freed */
}
```

The `filter` BPF program is stored in `cbs->filter` but never released. On the error path, the caller may not know whether the filter was consumed.

**Fix:** Clear `cbs->filter` on callback registration failure, or document ownership transfer point clearly. If the filter is owned by the caller until callback succeeds, do not store it until after successful registration:

```c
cbs->cb = rte_eth_add_first_rx_callback(port, qid, pdump_rx, cbs);
if (cbs->cb == NULL) {
	PDUMP_LOG_LINE(ERR, "failed to add rx callback, errno=%d", rte_errno);
	/* Do not store filter; caller retains ownership */
	return rte_errno;
}
cbs->filter = filter;
```

---

### 3. Same resource leak in `pdump_register_tx_callbacks()`

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_register_tx_callbacks()`

Identical issue: `filter` is stored in `cbs->filter` before callback registration. If `rte_eth_add_tx_callback()` fails, the filter leaks.

**Fix:** Apply the same fix as for RX callbacks.

---

### 4. Stale callback reconciliation does not wait for `inflight` drain

**File:** `lib/pdump/rte_pdump.c`  
**Functions:** `pdump_register_rx_callbacks()`, `pdump_register_tx_callbacks()`

When reconciling a stale callback, the code removes the old callback and waits for `use_count` to drain:

```c
ret = rte_eth_remove_rx_callback(port, qid, cbs->cb);
if (ret < 0) { /* ... */ }
pdump_cb_wait(cbs);
cbs->cb = NULL;
```

However, it does **not** wait for `ctl->inflight` to drain. If callbacks from the old generation are still executing (`inflight > 0`), the new enable can proceed and overwrite `cbs->ring`, `cbs->mp`, etc., while those old callbacks access the old values. This is a use-after-free if the old mempool/ring is freed after the first disable attempt timed out.

**Fix:** After `pdump_cb_wait(cbs)`, call `pdump_wait_inflight(ctl)` before re-enabling:

```c
pdump_cb_wait(cbs);
cbs->cb = NULL;

ret = pdump_wait_inflight(ctl);
if (ret < 0) {
	PDUMP_LOG_LINE(ERR, "timed out waiting for inflight drain during reconcile");
	return ret;
}
```

---

### 5. `enabled` flag set before callback registration completes

**File:** `lib/pdump/rte_pdump.c`  
**Functions:** `pdump_register_rx_callbacks()`, `pdump_register_tx_callbacks()`

The code sets `enabled = 1` immediately after successful callback registration:

```c
cbs->cb = rte_eth_add_first_rx_callback(port, qid, pdump_rx, cbs);
if (cbs->cb == NULL) { /* ... */ }

rte_atomic_store_explicit(&ctl->inflight, 0, rte_memory_order_relaxed);
rte_atomic_store_explicit(&ctl->enabled, 1, rte_memory_order_release);
```

However, the mempool, ring, and filter are stored in `cbs` **before** callback registration. If `rte_eth_add_first_rx_callback()` succeeds but another thread reads `enabled` before the registration completes, it could observe inconsistent state.

**Analysis:** This is marginal because the callback cannot be invoked until `rte_eth_add_first_rx_callback()` returns. The real issue is that `enabled` is set before error paths are fully handled. If a failure occurs after this point (not shown in current code), `enabled` would be left true.

**Recommendation:** Move the `enabled = 1` store to the very end of the successful path, after all error checks.

---

### 6. Missing error propagation in `__pdump_request()`

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `__pdump_request()`

When `pdump_request_to_secondary_sync()` returns a negative error after successful `set_pdump_rxtx_cbs()`, the primary process callbacks remain registered but the caller receives a failure. The primary's own callbacks are not rolled back.

```c
ret = set_pdump_rxtx_cbs(req);

if (ret == 0)
	ret = pdump_request_to_secondary_sync(req);

pdump_send_response(req, ret, bundle->peer);
```

If `pdump_request_to_secondary_sync()` fails (e.g., a secondary timed out), `ret` becomes negative and the client receives an error. However, the primary's own callbacks remain active. On a subsequent enable, the `reconcile stale callback` path will trigger, but in the meantime, packets may be captured on the primary but not on secondaries, leading to inconsistent state.

**Recommendation:** Document this behavior explicitly, or add a rollback path that disables the primary's callbacks if secondary propagation fails. The commit message says "no automatic RX/TX rollback on partial enable failure" is intentional, but this case is different--the primary succeeded, but the overall operation failed. At minimum, document this in the API.

---

### 7. `generation` increment can race with enable on another thread

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `__pdump_request()`

The generation increment during disable is:

```c
if (req->op == DISABLE) {
	req->generation = rte_atomic_fetch_add_explicit(&pdump_stats->generation, 1,
						   rte_memory_order_acq_rel) + 1;
}
```

If two threads call disable concurrently, both will increment `generation`. If a third thread calls enable between those two increments, it may read an intermediate generation value. The callbacks will then use mismatched generations, and the second disable may fail to wait for the first disable's callbacks to drain.

**Fix:** This is inherent to concurrent enable/disable without caller-side serialization. The patch should document that concurrent enable/disable on the same port/queue is unsupported, or add a per-port/queue lock to serialize operations.

---

## Warnings

### 1. Missing validation of `RTE_PDUMP_ALL_QUEUES` vs actual queue count

**File:** `lib/pdump/rte_pdump.c`  
**Functions:** `pdump_register_rx_callbacks()`, `pdump_register_tx_callbacks()`

The loops iterate `for (; qid < end_q; qid++)` where `end_q` is set by `set_pdump_rxtx_cbs()` based on `rte_eth_dev_info`. However, there is no validation that `nb_rx_q` and `nb_tx_q` are within `RTE_MAX_QUEUES_PER_PORT`. If a device reports more queues than the static array size, the loop will overflow the `rx_cbs`/`tx_cbs` arrays.

**Fix:** Add bounds checks:

```c
if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
	PDUMP_LOG_LINE(ERR, "device has more RX queues (%u) than supported (%u)",
			nb_rx_q, RTE_MAX_QUEUES_PER_PORT);
	return -ENOTSUP;
}
```

---

### 2. `pdump_cb_hold()`/`pdump_cb_release()` not called under generation check

**File:** `lib/pdump/rte_pdump.c`  
**Functions:** `pdump_rx()`, `pdump_tx()`

The `use_count` increment/decrement is performed **inside** the `pdump_enter()` check, but the filter stored in `cbs->filter` could theoretically be stale if the callback is invoked just after the generation changes but before `pdump_enter()` detects it.

**Analysis:** The `pdump_enter()` double-check with acquire ordering ensures that if `generation` has changed, the callback becomes a no-op before accessing `cbs->filter`. However, `pdump_cb_hold()` increments `use_count` which delays filter cleanup. This is safe because the filter is only freed after `pdump_cb_wait()` in disable, which waits for `use_count == 0`. So this is **not a bug**, but the ordering is subtle.

**Recommendation:** Add a comment in `pdump_cb_hold()` explaining why it's safe to increment `use_count` inside the generation-guarded region.

---

### 3. `rte_mp_request_sync_ex()` flag rejection prevents future extensibility

**File:** `lib/eal/common/eal_common_proc.c`  
**Function:** `rte_mp_request_sync_ex()`

The code rejects unknown flags:

```c
if (flags & ~RTE_MP_REQ_F_IGNORE_NO_ACTION) {
	rte_errno = EINVAL;
	goto end;
}
```

While the commit message states this is intentional, it means that adding a new flag in a future release will break applications compiled against an older DPDK. A more extensible approach would be to ignore unknown flags (they have no effect in this implementation) or return a dedicated error code.

**Recommendation:** Document in the API that unknown flags are rejected, and that applications should not pass flags they do not understand. Alternatively, consider ignoring unknown flags and returning success with a documented "best-effort" semantic.

---

### 4. `pdump_request_to_secondary_sync()` first-error aggregation loses per-secondary detail

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_request_to_secondary_sync()`

The loop aggregates errors by returning the first non-zero `resp->err_value`:

```c
if (resp->err_value != 0) {
	PDUMP_LOG_LINE(ERR, "secondary reply failed: op=%u err=%d",
		       resp->res_op, resp->err_value);
	if (ret == 0)
		ret = resp->err_value;
}
```

This provides only the first error to the caller. If multiple secondaries fail with different errors, only the first is returned. The commit message says "first-error aggregation" is documented, but this could be confusing if different secondaries report different failures (e.g., one times out, another returns `-ENOMEM`).

**Recommendation:** Consider returning a bitmask or array of per-secondary errors, or document more explicitly that only the first failure is returned and logs should be consulted for full details.

---

### 5. No bounds check on `mp_reply.nb_received`

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_request_to_secondary_sync()`

The loop iterates `for (i = 0; i < mp_reply.nb_received; i++)` without validating that `nb_received` is reasonable. If the underlying `rte_mp_request_sync_ex()` returns a corrupted reply structure (e.g., due to a bug or memory corruption), this could overflow.

**Fix:** Add a sanity check:

```c
if (mp_reply.nb_received > RTE_MAX_LCORE) {
	PDUMP_LOG_LINE(ERR, "invalid reply count %u", mp_reply.nb_received);
	ret = -EINVAL;
	goto cleanup;
}
```

---

### 6. Generation starts at 1 but zero is not documented as invalid

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `rte_pdump_init()`

The comment says "Start at generation 1 so zero remains the default pre-init value." However, there is no code that validates `generation != 0`. If a secondary initializes before the primary (which should not happen, but may due to misconfiguration), it could observe `generation == 0` and interpret it as valid.

**Recommendation:** Add a `RTE_BUILD_BUG_ON(sizeof(pdump_stats->generation) != sizeof(uint32_t))` or similar, and document that generation 0 is reserved/invalid.

---

### 7. `pdump_enter()` `enabled` check uses `relaxed` ordering

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_enter()`

The first check is:

```c
if (rte_atomic_load_explicit(&ctl->enabled, rte_memory_order_relaxed) == 0 ||
    cbs->generation != generation)
	return 0;
```

The `relaxed` ordering means that the callback could observe stale `enabled == 1` even after disable has set it to 0. The second check after incrementing `inflight` uses `acquire` and will eventually observe the updated value, so this is **safe** but adds latency.

**Recommendation:** Consider using `acquire` for the first `enabled` check to reduce the window. The current code is correct but may allow callbacks to spuriously increment `inflight` before observing the disable.

---

## Info

### 1. `pdump_cb_wait()` comment refers to "data loads"

**File:** `lib/pdump/rte_pdump.c`  
**Function:** `pdump_cb_wait()`

The comment says:

```c
/* make sure the data loads happens before the use count load */
rte_atomic_thread_fence(rte_memory_order_acquire);
```

This is unclear. The fence ensures that prior writes to `cbs` members (ring, mp, filter) are visible before the `use_count` load, but "data loads" is ambiguous. Consider rewording to "make sure writes to cbs members by