|WARNING| [v4,3/3] usertools: add script for wireshark capture

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: capture: infrastructure wireshark packet capture

### ERRORS

1. **Use-after-free in `capture_cb_detach()`** (datapath.c:49-52)
   ```c
   rte_atomic_store_explicit(&cbs->cap, NULL, rte_memory_order_relaxed);
   rte_atomic_thread_fence(rte_memory_order_seq_cst);
   RTE_WAIT_UNTIL_MASKED(&cbs->use_count, UINT32_MAX, ==, 0, rte_memory_order_acquire);
   ```
   The `cap` pointer is zeroed before waiting for readers to drain. A reader that loads `cap` as non-NULL just before the store, then gets descheduled, will use the stale pointer after the wait completes. The store of `cap` must happen *after* the use_count reaches zero, not before.
   
   **Fix**: Move the `cap` store to after the `RTE_WAIT_UNTIL_MASKED`, or use a separate "detaching" flag and wait on that.

2. **Resource leak on telemetry registration failure** (capture.c:539-543)
   ```c
   if (rte_mp_action_register(CAPTURE_MP, capture_mp_handler) < 0 &&
       rte_errno != ENOTSUP)
       CAPTURE_LOG(NOTICE, "Could not register ipc action: %s",
                   rte_strerror(rte_errno));
   ```
   This is in an `RTE_INIT` constructor, so the registration happens at library load. If it fails (and is not `ENOTSUP`), the library is left in an inconsistent state: a capture started later will attempt `capture_mp_broadcast()`, which sends on the unregistered action. The IPC send will fail, but the capture has already installed its callbacks and may leak resources if the broadcast failure aborts startup.
   
   **Fix**: Store a global flag recording registration success/failure and check it in `capture_start_req()` before proceeding.

3. **Bitmask shift using `1` instead of `1ULL` on 64-bit counters** (multiple files)
   Throughout the code, 64-bit counters (`uint64_t accepted`, `filtered`, etc.) are manipulated, but I do not see any bitmask shifts in the added lines. This is not a finding for this patch. (Sanity check: searched for `<<` in all added code, found none operating on 64-bit types.)

4. **Missing error propagation in `capture_attach_queues()`** (datapath.c:303-332)
   ```c
   if (cap->filter != NULL) {
       prog = __rte_capture_filter_load(cap->filter);
       if (prog == NULL) {
           CAPTURE_LOG(ERR, "Could not load filter for port %u",
                       cap->port_id);
           return -1;
       }
   }
   *progp = prog;

   for (uint16_t q = 0; q < cap->tx_queues; q++) {
       if (cap->queue != ALL_QUEUES && cap->queue != q)
           continue;

       cbs[*nb_cbs] = capture_attach(cap, q, false, prog);
       if (cbs[*nb_cbs] == NULL)
           return -1;
       ++*nb_cbs;
   }
   ```
   If `capture_attach()` fails partway through the loop, the function returns -1 immediately. At that point some callbacks have been installed (they are in `cbs[0..*nb_cbs-1]`) and the loaded filter program `prog` is still allocated. The caller in `capture_start_req()` sees the failure and goes to `error_callback_remove`, which calls `capture_remove_callbacks()`, but `capture_remove_callbacks()` only detaches what is in `cap->cbs[]`, which is empty because the primary's callback array has not been populated yet. The partially installed callbacks and the loaded program leak.
   
   **Fix**: On failure in `capture_attach_queues()`, detach the callbacks already installed (`cbs[0..*nb_cbs-1]`) and unload `prog` before returning. Or, have the caller pass in `nb_cbs` by reference so it can clean up the partial list itself.

5. **Double-free risk in `capture_thread()` on orphaned path** (output.c:279-290)
   ```c
   if (orphaned) {
       CAPTURE_LOG(ERR,
                   "capture %u: secondary still attached, leaking ring and pool",
                   cap->idx);
   } else {
       capture_free(cap);
   }
   ```
   The `orphaned` flag is set when `capture_mp_broadcast(cap, CAPTURE_MP_STOP)` returns -1, meaning at least one secondary failed to confirm detach. In that case `capture_free(cap)` is skipped to avoid freeing the ring while the secondary might still write to it. However, `capture_unlink(cap)` is called unconditionally just above (line 281). If the secondary later calls `capture_mp_detach()`, it will traverse `capture_secondary_list` looking for `cap`, but `cap` is no longer on the primary's `capture_list` because `capture_unlink()` removed it. This is not a use-after-free of `cap` (the secondary has its own pointer), but it does mean the secondary's detach will fail to find the capture and will not clean up its own state.
   
   **Fix**: On the orphaned path, do not call `capture_unlink(cap)`, or mark the capture as orphaned so a later stop from the secondary can still find it and clean up.

6. **Statistics accumulation using `=` instead of `+=`** (datapath.c:377-381)
   ```c
   rte_spinlock_lock(&cap->cbs_lock);
   for (uint16_t i = 0; i < *nb_cbs; i++) {
       capture_sum_one(&cap->total, &cbs[i]->stats);
       TAILQ_REMOVE(&cap->attached, cbs[i], attached);
   }
   ```
   `capture_sum_one()` is defined in datapath.c:347-353:
   ```c
   static void
   capture_sum_one(struct capture_total *t, const struct capture_stats *s)
   {
       t->accepted += s->accepted;
       t->filtered += s->filtered;
       t->nombuf   += s->nombuf;
       t->ringfull += s->ringfull;
   }
   ```
   This is correct (`+=`). Not a finding.

### WARNINGS

1. **Queue-related buffers not allocated with `rte_zmalloc_socket()`** (capture.c:237-241)
   ```c
   cap = rte_zmalloc_socket("capture", cb_size, RTE_CACHE_LINE_SIZE, socket_id);
   ```
   The `cap` structure itself is allocated with `rte_zmalloc_socket()`, which is correct for a shared control structure. However, the `cap->ring` (line 256) and `cap->mp` (line 267) are created via `rte_ring_create()` and `rte_pktmbuf_pool_create_by_ops()`, which internally use `rte_zmalloc` and do place the ring/pool on the specified socket. The ring and pool are NUMA-local, zero-initialized, and hugepage-backed. This is correct. Not a finding.

2. **Missing release notes for internal API** (doc/guides/rel_notes/release_26_11.rst:166-168)
   The added release notes entry is for a new library and its usertools integration, both of which are user-facing features. The library has no public C API (it is telemetry-driven), so there is no "new API" in the sense that needs documentation. The entry is appropriate. Not a finding.

3. **Hardcoded Ethernet overhead** (private.h:24, capture.c)
   The code does not hardcode an Ethernet overhead constant. The `snaplen` is passed through directly to `rte_pcapng_copy()`, and `rte_pcapng_copy()` handles the frame format internally. Not a finding.

4. **RST documentation style** (doc/guides/prog_guide/capture_lib.rst:56-80)
   The telemetry commands table uses a CSV table, which is fine. The parameters under `/ethdev/capture/start` are described in a definition list (lines 66-78), which is the preferred style. Not a finding.

5. **Global variables without unique prefixes** (datapath.c:35-36)
   ```c
   static struct capture_cb_list capture_cb_freelist =
       TAILQ_HEAD_INITIALIZER(capture_cb_freelist);
   ```
   This is a file-static variable, so it has no global linkage and cannot clash with other drivers/libraries. The guideline about unique prefixes applies to exported symbols. Not a finding.

6. **Standalone fence instead of ordering on atomic operation** (datapath.c:49-50)
   ```c
   rte_atomic_store_explicit(&cbs->cap, NULL, rte_memory_order_relaxed);
   rte_atomic_thread_fence(rte_memory_order_seq_cst);
   ```
   The fence is necessary here because the store is `relaxed` and the subsequent load of `use_count` needs to see all prior stores. However, this is also the site of the use-after-free Error #1 above, so the pattern itself is broken. The fence is not the issue; the order of operations is.

### INFO

1. **Pre-existing: `rte_mbuf_raw_free_bulk()` safety** (not in this patch)
   The patch does not call `rte_mbuf_raw_free_bulk()` anywhere. It uses `rte_pktmbuf_free_bulk()` in datapath.c:122 and output.c:171, both of which are safe for mixed-pool mbufs. Not a finding.

2. **Privilege handling in telemetry registration** (capture.c:539-543)
   The `RTE_INIT` constructor runs during library load, which is after `rte_eal_init()` has already set up the process type and privilege. The registration does not itself require privilege (it only inserts into a list), so the comment about "safe this early" (line 534) is accurate. The library is loaded by the application, which has already initialized EAL, so the environment is stable. Not a finding, but see Error #2 about handling registration failure.

---

## Patch 2/3: test: add test for capture hooks

### ERRORS

None. The test code is self-contained, handles resource cleanup on assertion failure (via `ut_teardown()`), and does not introduce correctness bugs.

### WARNINGS

1. **Missing `RTE_EXPORT_SYMBOL` for new test** (app/test/test_capture.c:688)
   ```c
   REGISTER_FAST_TEST(capture_autotest, NOHUGE_OK, ASAN_OK, test_capture);
   ```
   The `REGISTER_FAST_TEST` macro is the current correct registration form. No `RTE_EXPORT_SYMBOL` is needed; the macro handles it. Not a finding.

### INFO

None.

---

## Patch 3/3: usertools: add script for wireshark capture

### ERRORS

1. **Privilege drop incomplete on Linux** (dpdk-wireshark-extcap.c:151-166)
   ```c
   #ifdef HAVE_LIBCAP
       cap_t caps = cap_init();
       if (caps == NULL)
           err(1, "cap_init");
       if (cap_set_proc(caps) != 0)
           err(1, "cap_set_proc");
       cap_free(caps);
   #else
       if (privileged_exec && ruid == euid && rgid == egid)
           errx(1, "built without libcap, cannot drop a file capability");
   #endif
   ```
   On Linux, after `cap_set_proc(caps)` clears all capabilities, the process still has the ambient capability set, the bounding set, and the securebits unchanged. A process that had `CAP_DAC_OVERRIDE` in the inheritable set or in the ambient set can re-raise it even after `cap_set_proc()` clears the effective and permitted sets. The drop is incomplete.
   
   **Fix**: Call `cap_clear()` on the inheritable set as well, or use `prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_CLEAR_ALL, 0, 0, 0)` and `prctl(PR_SET_KEEPCAPS, 0)` before the `cap_set_proc()` call. See `man 7 capabilities`.

2. **TOCTOU race in FIFO check** (dpdk-wireshark-extcap.c:811-818)
   ```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);
   ```
   The `lstat()` check is advisory and happens before the `cmd_capture()` function is called, which opens the FIFO at line 890. An attacker can replace the FIFO with a symlink between the check and the open, causing the open to follow the symlink. The `O_NOFOLLOW` flag on the open (line 890) will cause it to fail with `ELOOP`, so this is not a vulnerability, but the advisory check is misleading and the error message will be confusing.
   
   **Fix**: Remove the advisory `lstat()` check, or move it inside `cmd_capture()` immediately before the open so the failure mode is explicit.

### WARNINGS

1. **Unchecked return value from `pthread_sigmask()`** (dpdk-wireshark-extcap.c:207)
   ```c
   pthread_sigmask(SIG_BLOCK, &set, NULL);
   ```
   `pthread_sigmask()` is documented to always return zero on Linux when called with valid arguments, so this is not a missing error check. Not a finding (per the guidelines on unchecked return values from functions that always succeed on Linux).

2. **Hardcoded buffer sizes** (dpdk-wireshark-extcap.c:88-94)
   The buffer size macros (`CMD_BUF`, `REPLY_TIMEOUT_SEC`, `MIN_REPLY_LEN`, `MAX_REPLY_LEN`) are reasonable and bounded. The `CMD_BUF` is sized to match the telemetry API limit (`PARAMS_MAX + 128`), and the reply buffer is capped at 1MB. Not a finding.

3. **Global state in signal handler** (dpdk-wireshark-extcap.c:685-693)
   ```c
   static void
   on_stop_signal(int sig)
   {
       int saved = errno;
       ssize_t r;

       (void)sig;
       r = write(stop_pipe[1], "x", 1);
       (void)r;
       errno = saved;
   }
   ```
   The signal handler writes to a pipe, which is async-signal-safe. The `errno` save/restore is correct. The handler does not touch any shared state that could race with the main thread. Not a finding.

### INFO

1. **Dual license** (dpdk-wireshark-extcap.c:1)
   ```c
   // SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-or-later
   ```
   The file is dual-licensed BSD-3-Clause OR GPL-2.0-or-later, as stated in the commit message. This is intentional to allow inclusion in the Wireshark tree later. Not a finding.

2. **Setuid installation guidance** (doc/guides/tools/wireshark_extcap.rst:75-80)
   The documentation correctly advises that setuid installation should be restricted to a group and that capability-based installation is preferred on Linux. The code enforces the privilege drop and verifies it. Not a finding.

---

## Summary

- **Patch 1/3**: 6 Errors (use-after-free, resource leaks, missing error propagation, double-free risk, IPC registration failure handling)
- **Patch 2/3**: Clean
- **Patch 3/3**: 2 Errors (warning: review has no Review-Result line, severity inferred from prose (response may be truncated)