|WARNING| [v5,4/4] usertools: add script for wireshark capture

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

I'll review this DPDK patch series against the AGENTS.md guidelines, focusing on correctness bugs, C coding style, API requirements, and other violations.

## PATCH 1/4: capture library infrastructure

### ERRORS

**Error 1: Resource leak on error path (capture.c:428)**
```c
if (rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_DESTROY,
                  capture_port_destroy, NULL) != 0) {
    capture_err(d, "can not register for port destroy events");
    goto error_free;
}

if (capture_add_callbacks(cap) < 0) {
    capture_err(d, "can not register callbacks");
    goto error_callback_remove;
}
```
The `error_callback_remove` label unregisters the callback but the `error_free` path does not. If `capture_add_callbacks()` fails, the port destroy callback registered at line 428 is leaked.

**Fix**: The `error_free` label should unregister the callback before freeing.

**Error 2: Missing bounds check on user-supplied index (capture.c:137-141)**
```c
if (parse_uint32(args[0], &port_id, RTE_MAX_ETHPORTS) < 0) {
    capture_err(d, "invalid port_id='%s'", args[0]);
    return -1;
}
```
Then later:
```c
if (rte_eth_dev_info_get(cfg.port_id, &dev_info) < 0) {
    capture_err(d, "can not get info for port %u", cfg.port_id);
    return 0;
}
```
The `parse_uint32` accepts values up to but not including `RTE_MAX_ETHPORTS` (via the `< limit` test), but `rte_eth_dev_info_get()` expects a port_id from the valid device list. A port_id that is in range but refers to a device that was never created or has been released would pass validation but fail at info_get. While info_get does catch it, the error message is misleading. Consider validating with `rte_eth_dev_is_valid_port()` after the range check.

**Error 3: Secondary attachment failure handling incomplete (multiproc.c:231-240)**
```c
if (resp->err != 0 && resp->err != -ENOENT &&
    resp->err != -EPERM) {
    CAPTURE_LOG(ERR, "Secondary could not %s capture: %s",
            op == CAPTURE_MP_START ? "start" : "stop",
            rte_strerror(-resp->err));
    ret = -1;
}
```
When a secondary fails to attach on START but the function returns -1, the primary has already installed its own callbacks and created the capture thread (capture.c:463-467). The caller at capture.c:448 sees the failure but the callbacks and thread are already running. The cleanup path `error_secondary_stop` broadcasts STOP, but if that also fails (which it will if the same secondary is still broken), the capture is left in the `orphaned` state and leaks the ring/pool.

**Fix**: The START broadcast failure should stop the capture before returning to the caller, not rely on the caller to do it.

### WARNINGS

**Warning 1: `rte_malloc` used for ordinary control structure (capture.c:243-249)**
```c
cap = rte_zmalloc_socket("capture", cb_size, RTE_CACHE_LINE_SIZE, socket_id);
```
This structure is not accessed by DMA, not shared with secondary processes (the IPC passes the pointer but each process has its own copy via `rte_zmalloc`), and NUMA placement for control-path structures is not critical. Use standard `malloc()` instead; see the "Appropriate Use of rte_malloc()" section of AGENTS.md.

Note: The ring and mempool allocated later (lines 255, 268) are correctly using `rte_malloc` equivalents because they are shared memory.

**Warning 2: Boolean variable uses int (capture.c:308)**
```c
rte_tel_data_add_dict_int(d, "running",
              rte_atomic_load_explicit(&cap->running,
                           rte_memory_order_relaxed));
```
`cap->running` is declared as `RTE_ATOMIC(bool)` in private.h:88, but telemetry has no `add_dict_bool()`, so this cast to int is unavoidable. Not a bug, but worth noting that the telemetry API forces this.

**Warning 3: Queue-related buffers should use `rte_zmalloc_socket` (datapath.c:199-204)**
```c
cbs = rte_zmalloc_socket("capture_cb", sizeof(*cbs),
             RTE_CACHE_LINE_SIZE, cap->socket_id);
```
This is correct: queue control structures should use `rte_zmalloc_socket()` for NUMA locality and hugepage backing (secondary process visibility). Well done.

**Warning 4: Filter program kept in process-private memory (filter.c:121-128)**
```c
prog = calloc(1, sizeof(*prog));
if (prog == NULL) {
    CAPTURE_LOG(ERR, "could not allocate filter program");
    return NULL;
}
```
The per-process loaded form of the filter uses `calloc()` rather than `rte_malloc()`. This is correct: the JIT output and `rte_bpf` object are process-private mappings per the comment at filter.c:33-39. No issue here.

**Warning 5: Missing release notes for internal helpers**
Several internal functions (e.g. `capture_rcu_init`, `capture_mp_register`) have no Doxygen and are not part of the public API. Release notes correctly cover only the telemetry commands and the library itself. No issue.

### INFO

**Info 1: Function pointer array not const (datapath.c, if applicable)**
Checked: no function pointer arrays found in this patch. The callbacks are registered individually via `rte_eth_add_rx_callback()`/`rte_eth_add_tx_callback()`, not via a dispatch table. No issue.

**Info 2: Statistics counter updates (datapath.c:100-103)**
```c
stats->accepted += d_pkts;
```
Correctly uses `+=` for accumulation. Well done.

**Info 3: RCU implementation choices (rcu.c)**
The custom RCU uses `membarrier()` as documented. The choice to implement a library-specific RCU rather than use `lib/rcu` is explained in the commit message: QSBR requires application cooperation, which defeats the point of a transparent capture library. The rationale is sound.

**Info 4: Scatter Rx and MTU handling**
Checked throughout: no MTU vs frame length confusion, no scatter Rx decision based on raw MTU without overhead. The library operates at the callback level after the PMD has already made those decisions. No issue.

**Info 5: Pre-existing: capture_port_destroy stops multiple captures in batches**
(capture.c:337-361) The port destroy handler stops captures in batches of `CAPTURE_STOP_BATCH` to overlap their flushes. This is an optimization, not a bug, and is pre-existing to this review. Noted for completeness.

## PATCH 2/4: --no-capture EAL option

### ERRORS

None.

### WARNINGS

**Warning 1: API addition without `__rte_experimental` (rte_eal.h:400-401)**
```c
int rte_eal_has_capture(void);
```
New public API function added without `__rte_experimental` marker. Per AGENTS.md, new API functions must be marked experimental.

**Fix**: Add `__rte_experimental` on the line immediately before the return type:
```c
__rte_experimental
int rte_eal_has_capture(void);
```

**Warning 2: Missing `RTE_EXPORT_SYMBOL` macro in .c file (eal_common_config.c:113-117)**
```c
RTE_EXPORT_SYMBOL(rte_eal_has_capture)
int
rte_eal_has_capture(void)
{
    return !internal_config.no_capture;
}
```
The function is exported via the macro, but since it is experimental, it should use `RTE_EXPORT_EXPERIMENTAL_SYMBOL(name, version)` instead of `RTE_EXPORT_SYMBOL`. Version should be the release this is added in, e.g. `25.03` if targeting the 25.03 release.

### INFO

**Info 1: EAL option naming consistency**
The option `--no-capture` follows the pattern of `--no-telemetry`, `--no-pci`, etc. Consistent with existing EAL options. No issue.

## PATCH 3/4: test suite for capture

### ERRORS

None. The test suite is well-structured and correctly uses the `TEST_ASSERT` macros and `unit_test_suite_runner`.

### WARNINGS

**Warning 1: Test-only change, no release notes required**
The MAINTAINERS update and test additions do not require release notes per AGENTS.md: "Release notes are NOT required for test-only changes." The release notes in patch 1/4 already cover the library addition. No action needed, but worth noting for completeness.

### INFO

**Info 1: Test suite structure**
The test correctly uses `unit_test_suite_runner` infrastructure, `TEST_ASSERT` macros, and `REGISTER_FAST_TEST`. Well done.

**Info 2: net_null driver dependency**
The test depends on `net_null` and is skipped if unavailable (meson.build line 44). This is the correct approach per AGENTS.md.

## PATCH 4/4: dpdk-wireshark-extcap tool

### ERRORS

**Error 1: Privilege drop verification incomplete (dpdk-wireshark-extcap.c:152-161)**
```c
if (getuid() != ruid || geteuid() != ruid ||
    getgid() != rgid || getegid() != rgid)
    errx(1, "privilege drop failed");
if (euid != ruid && seteuid(euid) == 0)
    errx(1, "privilege drop failed");
if (egid != rgid && setegid(egid) == 0)
    errx(1, "privilege drop failed");
```
The verification attempts to raise privilege back to prove it cannot. However, on Linux with file capabilities, `euid == ruid` throughout (no setuid), so the `seteuid(euid)` test is skipped. The capability drop at line 172 is what matters, but if `libcap` is present but `cap_set_proc()` silently fails (e.g. due to a kernel bug or seccomp filter), privilege may not actually be dropped.

**Fix**: After `cap_set_proc()`, read the capability set back with `cap_get_proc()` and verify it matches the all-off set, rather than just checking the return value.

**Error 2: Potential TOCTOU race (dpdk-wireshark-extcap.c:873-880)**
```c
if (lstat(o.fifo, &sb) != 0)
    err(1, "%s", o.fifo);
if (!S_ISFIFO(sb.st_mode))
    errx(1, "%s is not a fifo", o.fifo);
```
Then later at line 920:
```c
fifo_fd = open(fifo, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
if (fifo_fd < 0)
    err(1, "cannot open fifo %s", fifo);
if (fstat(fifo_fd, &sb) != 0 || !S_ISFIFO(sb.st_mode))
    errx(1, "%s is not a fifo", fifo);
```
The `lstat()` at line 873 is advisory ("fail before costing the primary a connection"), but the real check is the `fstat()` after `open()`. However, between the `lstat()` and the privilege drop at line 890, the file could be replaced. This is low severity because the `fstat()` after `open()` is the real defense, and `O_NOFOLLOW` prevents symlink attacks, but the advisory check could be removed entirely to avoid confusion.

**Recommendation**: Remove the advisory `lstat()` check, or move it to just before `open()` if keeping it.

### WARNINGS

**Warning 1: Missing Doxygen for privilege-related functions**
Functions like `priv_init()`, `priv_drop()`, `exec_was_privileged()` have no Doxygen comments. Since this is a standalone tool not a library, and these are internal functions, this is Info rather than Warning. However, given the security sensitivity, brief comments would be valuable.

**Warning 2: Environment ignored after privileged exec**
(Line 182, 372) The comment says "Ignored, along with `XDG_RUNTIME_DIR`, after a privileged start" but the code at line 372 only checks `!privileged_exec` for `DPDK_EXTCAP_PATH`, not for `XDG_RUNTIME_DIR`. The code at line 381 does not guard `getenv("XDG_RUNTIME_DIR")`.

**Fix**: Either guard the `XDG_RUNTIME_DIR` read at line 381 with `!privileged_exec`, or clarify the comment to say only `DPDK_EXTCAP_PATH` is ignored.

### INFO

**Info 1: Standalone tool, dual-licensed**
The tool is dual-licensed BSD-3-Clause OR GPL-2.0-or-later to allow inclusion in Wireshark. This is appropriate and well-documented. No issue.

**Info 2: No DPDK library dependencies**
The tool speaks the telemetry protocol directly and links against nothing from DPDK. This is by design to allow setuid/capability installs without linking DPDK as root. No issue.

**Info 3: Privilege drop follows dumpcap pattern**
The privilege handling follows the Wireshark `dumpcap` model: raise privilege only for the connect, drop permanently before parsing any reply. This is a sound security design. The verification issue in Error 1 above should be addressed, but the overall approach is correct.

## SUMMARY

**Errors:**
1. Patch 1: Resource leak on error path (capture.c:428)
2. Patch 1: Secondary attachment failure handling incomplete (multiproc.c:231-240)
3. Patch 4: Privilege drop verification incomplete (dpdk-wireshark-extcap.c:152-161)
4. Patch 4: TOCTOU race (dpdk-wireshark-extcap.c:873, advisory check)

**Warnings:**
1. Patch 1: `rte_malloc` used for ordinary control structure
2. Patch 2: API addition without `__rte_experimental` marker
3. Patch 2: Missing `RTE_EXPORT_EXPERIMENTAL_SYMBOL`
4. Patch 4: Environment handling inconsistency (XDG_RUNTIME_DIR)

**Info:**
- Patch 1: Several design choices are sound and well-documented
- Patch 3: Test suite is well-structured
- Patch 4: Privilege handling follows dumpcap pattern (with one verification gap)

Review-Result: ERROR