|WARNING| [v2] examples/rpcapd: demo version of packet capture daemon

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-20

# DPDK Patch Review

Reviewing patch: `[PATCH v2] examples/rpcapd: demo version of packet capture daemon`

---

## Summary

This patch adds a new example application `dpdk-rpcapd` that implements a remote packet capture daemon using the RPCAP protocol. The application runs as a DPDK secondary process and allows libpcap-aware tools (tcpdump, Wireshark) to capture packets from DPDK ports live.

---

## Errors

### 1. `handle_open()` - Resource leak on error path after `stop_capture()`

**File:** `examples/rpcapd/main.c:447`

```c
if (rte_eth_dev_get_port_by_name(s->name, &port) < 0) {
    RPCAPD_LOG(WARNING, "open: no such port '%s'", s->name);
    /* s->name has already been overwritten; make sure a later
     * STARTCAP cannot capture the previously opened port.
     */
    s->opened = false;
    return rpcap_send_error(fd, 0, "unknown interface");
}
```

After the call to `rte_eth_dev_get_port_by_name()` fails, the function returns after sending an error. However, if a previous `handle_open()` succeeded and then this one fails, `stop_capture()` has already been called at the start of the function (line 421), which frees `s->ring`, `s->mp`, `s->prm`, and closes `s->data_fd`. The error path here is correct in that regard.

**Wait, re-reading the code**: The call to `stop_capture(s)` happens at line 421, unconditionally. So by the time we reach the error at line 447, any prior resources have already been freed. The comment "Unconditionally, not just when capture_on" confirms this is intentional. So this is actually correct.

No issue here.

### 2. `handle_startcap()` - `rte_free(s->prm)` after `stop_capture()` already freed it

**File:** `examples/rpcapd/main.c:632-635`

```c
struct rte_bpf_prm *recorded;
...
recorded = s->prm;
s->prm = NULL;
stop_capture(s);
s->prm = recorded;
```

The function saves `s->prm` in `recorded`, sets `s->prm` to NULL, calls `stop_capture()`, then restores `s->prm = recorded`. Inside `stop_capture()` (line 586):

```c
rte_free(s->prm);
s->prm = NULL;
```

Because `s->prm` was set to NULL before calling `stop_capture()`, `rte_free(NULL)` is called, which is safe (rte_free handles NULL). After `stop_capture()`, `s->prm` is restored to `recorded`. So the filter survives the `stop_capture()` and is reused. This is correct.

On the error paths in `handle_startcap()` where `stop_capture(s)` is called again (lines 697, 710, 715, 726), `s->prm` still points to `recorded`, and `stop_capture()` will `rte_free(s->prm)`. So `recorded` is freed on those error paths, which is correct because the function is failing and the filter cannot be used.

No issue here.

### 3. `read_filter()` - Double-free of `s->prm` on error after allocation

**File:** `examples/rpcapd/main.c:568-572`

```c
rte_free(s->prm);
s->prm = rte_bpf_convert(&bf);
free(insns);
if (s->prm == NULL) {
    RPCAPD_LOG(ERR, "rte_bpf_convert failed: %s",
        rte_strerror(rte_errno));
    return rpcap_send_error(fd, 0, "cannot convert filter") < 0 ? -1 : 1;
}
```

If `rte_bpf_convert()` fails and returns NULL, `s->prm` is set to NULL, and the function returns. The comment at line 560 says "A filter recorded by an earlier UPDATEFILTER may still be here; it is about to be replaced, so do not leak it." The `rte_free(s->prm)` at line 568 frees any existing filter before allocating the new one. If `rte_bpf_convert()` fails, `s->prm` is NULL, so no leak occurs. This is correct.

No issue here.

### 4. `handle_updatefilter()` - `s->prm` restored to `old` on error, but `old` may be stale

**File:** `examples/rpcapd/main.c:743-750`

```c
struct rte_bpf_prm *old = s->prm;
int ret;

s->prm = NULL;
ret = read_filter(fd, plen, s);
if (ret != 0) {
    /* Malformed request: keep running with the old filter. */
    rte_free(s->prm);
    s->prm = old;
    return ret < 0 ? -1 : 0;	/* error already reported */
}
```

If `read_filter()` fails, `s->prm` (the new filter) is freed, and `s->prm` is restored to `old`. Then the function returns. On a later call to `handle_updatefilter()` or `stop_capture()`, `s->prm` (which now points to `old`) will be freed again. This is a **double-free**: `old` was never set to NULL after being saved, and if `read_filter()` fails, `old` is restored and will be freed twice.

**Wait, let me re-check**: `old = s->prm` saves the original filter. Then `s->prm = NULL`. If `read_filter()` succeeds, it allocates a new filter and sets `s->prm` to it. If `read_filter()` fails, `s->prm` is freed (but it might be NULL if `read_filter()` didn't allocate anything, or it might be a partial allocation). Then `s->prm = old` restores the original filter pointer.

Actually, looking at `read_filter()`: if it fails after `s->prm = rte_bpf_convert(&bf)`, `s->prm` is either NULL (convert failed) or a valid pointer (convert succeeded but a later error occurred). But `read_filter()` doesn't free `s->prm` on all error paths. Let me check:

- Line 572: if `rte_bpf_convert()` returns NULL, `s->prm` is NULL, and the function returns. No free needed.
- Line 549, 554: `rpcap_send_error()` returns, but `s->prm` hasn't been set yet.
- Line 558: `s->prm` is allocated inside `rte_bpf_convert()` but if validation fails after that... wait, there's no validation after `rte_bpf_convert()`. The validation happens at line 556 before the convert.

So if `read_filter()` returns an error, `s->prm` is either NULL or a valid newly-allocated filter. In `handle_updatefilter()`, the code does `rte_free(s->prm)` then `s->prm = old`. So if `s->prm` is NULL, `rte_free(NULL)` is safe. If `s->prm` is a valid new filter, it gets freed, then `old` is restored. This is correct.

**But wait**: In `handle_startcap()`, the code does:

```c
recorded = s->prm;
s->prm = NULL;
stop_capture(s);
s->prm = recorded;
```

So `recorded` is the filter that was set by a previous `UPDATEFILTER`. Then `stop_capture()` is called, which does `rte_free(s->prm)` (but `s->prm` is NULL, so no-op). Then `s->prm = recorded` restores it. Then `read_filter()` is called, which does `rte_free(s->prm)` (freeing `recorded`) before allocating a new one. So `recorded` is freed inside `read_filter()` if a filter is being set.

OK so the pattern is:
- `read_filter()` frees `s->prm` before allocating a new one (line 568).
- If `read_filter()` fails, `s->prm` is either NULL or a newly-allocated filter that should be freed.

In `handle_updatefilter()`:
- `old = s->prm` saves the current filter.
- `s->prm = NULL`.
- `read_filter()` is called. It does `rte_free(s->prm)` (no-op because NULL), then allocates a new filter.
- If `read_filter()` fails, `s->prm` is freed, then `s->prm = old`.

So the current filter is saved in `old`, then cleared. `read_filter()` tries to allocate a new one. If it fails, the new one is freed, and the old one is restored. The old one is still valid because it was saved before being cleared. This is correct.

No issue here.

### 5. `capture_loop()` - Missing check for `s->data_fd` validity before calling `send_iov_full()`

**File:** `examples/rpcapd/main.c:908-911`

Inside `process_ring()`, line 910:

```c
if (send_iov_full(s->data_fd, iov, 3, more ? MSG_MORE : 0) < 0) {
```

If `s->data_fd` is -1 (e.g., if `accept_timeout()` failed in `handle_startcap()`), this will call `sendmsg()` on fd -1, which will return `EBADF`. The code checks for errors and returns -1, which is correct. But it's better to avoid the syscall. However, if `handle_startcap()` failed, `s->capture_on` would be false, and `capture_loop()` wouldn't be called. So this is safe.

No issue here.

---

## Warnings

### 1. Missing release notes for internal/test-only changes

**File:** `doc/guides/rel_notes/release_26_11.rst`

Release notes are included for this example application. The guidelines state that release notes are NOT required for test-only changes or internal API. However, this is a new example application (a user-facing tool), so release notes are appropriate. This is correct.

No issue here.

### 2. New API not marked as `__rte_experimental`

The patch does not add any new public API functions. It is an example application that uses existing DPDK APIs (`rte_pdump`, `rte_bpf`, etc.). The `rpcap_*` functions are internal to the example and not exported. No new library API is added.

No issue here.

### 3. `rte_malloc()` used for general allocations

The code uses `rte_malloc()` in `create_capture_mempool()` and `create_capture_ring()`, but these are DPDK objects created via `rte_pktmbuf_pool_create()` and `rte_ring_create()`, which allocate from hugepage memory internally. The example does not call `rte_malloc()` for general control structures. Standard `malloc()`/`calloc()`/`realloc()` are used for buffers that do not need hugepage backing (e.g., line 378, 534). This is correct.

No issue here.

### 4. Statistics accumulation pattern

At line 834 in `process_ring()`:

```c
s->npkt++;
```

This is an increment, not an assignment. Correct.

At line 1037 in `handle_stats()`:

```c
if (s->capture_on)
    rte_eth_stats_get(s->port, &es);
```

`rte_eth_stats_get()` fills the structure with absolute values from the hardware, not incremental updates. The code then uses those values directly in the reply. This is correct (it's not a running counter in the daemon, it's a snapshot from the NIC).

No issue here.

### 5. Queue-related buffers allocated with `rte_zmalloc_socket()`

`create_capture_ring()` uses `rte_ring_create()`, which internally uses hugepage memory. `create_capture_mempool()` uses `rte_pktmbuf_pool_create()`, which also uses hugepage memory. Both are appropriate for shared-memory objects that secondary processes can access. The functions are correct.

No issue here.

### 6. RST documentation style

The RST documentation in `doc/guides/sample_app_ug/rpcapd.rst` uses bullet lists for options (lines 54-104). These are short option descriptions, not term/definition pairs. The format is appropriate.

The "Limitations" section (lines 176-204) uses bullet lists where each item is a short phrase or sentence. This is also appropriate.

No issue here.

### 7. Missing tests

This is an example application demonstrating a protocol implementation. It is not a library or PMD. Example applications typically do not have unit tests in `app/test`. The documentation includes usage examples. This is acceptable.

No issue here.

---

## Info

### 1. `bool` usage

The code uses `bool` for boolean flags (`opened`, `capture_on`, `promisc_set`, `ipv4_only`, etc.). This is correct and follows the guidelines.

### 2. Atomic usage for `quit_signal`

At line 158:

```c
static RTE_ATOMIC(bool) quit_signal;
```

The signal handler (line 162) writes to it with `rte_atomic_store_explicit(..., rte_memory_order_relaxed)`, and the main loop (line 176 and elsewhere) reads it with `rte_atomic_load_explicit(..., rte_memory_order_relaxed)`. This is correct usage of atomics for a simple flag.

### 3. Function pointer array

No function pointer arrays are present in this example.

### 4. Process-shared synchronization

No pthread mutexes, condition variables, or rwlocks are used in this code. The example uses DPDK rings and atomics, which are safe for shared memory. No issue here.

### 5. Naming conventions

All DPDK symbols use `rte_` or `RTE_` prefixes (from the libraries). The example's own functions (`handle_*`, `rpcap_*`, `open_listen_socket`, etc.) are `static` and not exported, so they do not need the prefix. This is correct.

### 6. Cryptographic patterns

No cryptographic code is present in this example.

### 7. Correctness patterns

- **Use-after-free**: I reviewed `stop_capture()` and its callers. Resources are freed and pointers are set to NULL. No use-after-free detected.
- **Resource leaks**: All error paths in `handle_startcap()` call `stop_capture()` to clean up. `stop_capture()` is safe to call multiple times because it checks and NULLs pointers. No leaks detected.
- **Error checks**: All `recv_full()`, `rpcap_send_msg()`, and allocation calls are checked. Correct.
- **Integer overflow**: The `ring_size` is bounded by `MAX_RING_SIZE` (1U << 20) before being passed to `rte_ring_create()`. The `rte_align32pow2()` usage is guarded by `rte_is_power_of_2()` and an upper-bound check. Correct.
- **MTU confusion**: Not applicable (no MTU handling in this example).

---

## Conclusion

**No errors found.**

This patch is well-structured and follows DPDK coding standards. The use of atomics, resource management, and error handling is correct. The documentation is clear and appropriate for an example application.

**Recommendation: APPROVE**