|SUCCESS| [v2,12/12] interrupts: warn on leaked event FDs

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/172050

_AI Code Review_

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

# DPDK Patch Series Review

Reviewing 12 patches in this series addressing interrupt handle file descriptor management and resource leaks.

---

## Patch 1/12: eal/linux: fix FD leak in alarm subsystem

**Changes:**
- Adds `rte_intr_fd_close(intr_handle)` before `rte_intr_instance_free(intr_handle)` in cleanup path

**Findings:**

### Errors

None.

### Warnings

None.

### Info

- Pre-existing: The patch targets `lib/eal/linux/eal_alarm.c` but there is a symmetric code path in `lib/eal/freebsd/eal_alarm.c` (FreeBSD implementation) that was already fixed according to the commit message. The Linux version should have been fixed at the same time but was missed.

---

## Patch 2/12: eal/linux: reset FD in interrupt handle for uevent

**Changes:**
- Replaces `close(rte_intr_fd_get(intr_handle))` with `rte_intr_fd_close(intr_handle)` in two locations in `lib/eal/linux/eal_dev.c`

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 3/12: interrupts: do not duplicate interrupt handles

**Changes:**
- Removes `rte_intr_instance_dup()` function
- Changes interrupt sources to store `const struct rte_intr_handle *` instead of owned copies
- Updates `rte_intr_unregister_callback_fn` signature to take `const struct rte_intr_handle *`
- Switches from FD-based to pointer-based source matching in register/unregister paths
- Updates memif driver's unregister callback to use the stored handle from `cc->intr_handle` instead of the parameter

**Findings:**

### Errors

None.

### Warnings

None.

### Info

- The change from FD-based to pointer-based source matching enforces the existing invariant that one intr_handle maps to one FD. The commit message explains this resolves a pre-existing issue where different handles with the same FD would share a source and receive the wrong handle in callbacks.
- The signature change to `const` in the unregister callback is an API change correctly documented in `doc/guides/rel_notes/release_26_11.rst`.

---

## Patch 4/12: bus/pci: reset VFIO device FD after release

**Changes:**
- Adds `rte_intr_dev_fd_set(dev->intr_handle, -1)` after `rte_vfio_release_device()` in both primary and secondary unmap functions
- Removes setting `vfio_req_intr_handle` dev_fd in secondary process (it was never used there)

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 5/12: net/mana: reset FD in interrupt handle

**Changes:**
- Adds `rte_intr_fd_set(priv->intr_handle, -1)` before `rte_intr_instance_free()` in two locations, with comments noting the FD is owned by ibverbs

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 6/12: net/memif: fix some interrupt FD leaks

**Changes:**
- In `memif_socket_create()`: changes early return on `rte_intr_callback_register()` failure to jump to the error path
- In `memif_socket_init()`: adds cleanup when `rte_hash_add_key_data()` fails, including unregistering callback, closing FD, and freeing handle for listener sockets
- In `memif_socket_remove_device()`: adds unregister, close, and FD reset before freeing handle

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 7/12: net/memif: reset FD in interrupt handle

**Changes:**
- In `memif_listener_handler()` error path: adds `rte_intr_fd_set(cc->intr_handle, -1)` with comment that FD is closed separately
- In `memif_socket_create()` error path: same pattern
- In `memif_socket_init()`: replaces separate close + set with `rte_intr_fd_close()`
- In `memif_socket_remove_device()`: same
- In `memif_connect_client()`: adds FD reset in error path with comment

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 8/12: net/sxe2: reset FD in interrupt handle

**Changes:**
- Adds `rte_intr_fd_set(tmp_intr_handle, -1)` before `rte_intr_instance_free()` in `sxe2_intr_handler_create()` error path, with comment that FD is not owned

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 9/12: raw/ifpga: reset FD in interrupt handle

**Changes:**
- Adds `rte_intr_fd_set(*intr_handle, -1)` before `rte_intr_instance_free()` in `ifpga_unregister_msix_irq()`, with comment noting FD was pointing to efd[0] already closed by `rte_intr_efd_disable()`

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 10/12: eal/linux: fix closing of event FDs not owned by EAL

**Changes:**
- In `rte_intr_efd_disable()`: replaces the condition `rte_intr_max_intr_get(intr_handle) > rte_intr_nb_efd_get(intr_handle)` with an explicit check `rte_intr_type_get(intr_handle) == RTE_INTR_HANDLE_VFIO_MSIX` to determine ownership

**Findings:**

### Errors

None.

### Warnings

None.

### Info

- The commit message explains that the old heuristic (max_intr > nb_efd) incorrectly identified ownership. virtio-user fills the event FD array for VDEV handles with the same max_intr value, causing EAL to close FDs it does not own, leading to double-close when the driver later closes them. The fix uses an explicit type check, which is more direct and correct.

---

## Patch 11/12: eal/linux: fix eventfd leaks when enabling interrupts

**Changes:**
- In `rte_intr_efd_enable()` for VFIO_MSIX:
  - Adds check at the start: if `nb_efd != 0`, return `-EBUSY` (reject repeated enable without disable)
  - Updates `nb_efd` as each eventfd is created (previously only set after all succeeded)
  - On eventfd creation or set failure, closes the just-created FD, clears its entry, and jumps to error path
  - Error path closes all eventfds created so far, clears all entries, and resets `nb_efd` to 0

**Findings:**

### Errors

None.

### Warnings

None.

---

## Patch 12/12: interrupts: warn on leaked event FDs

**Changes:**
- In `rte_intr_instance_free()`: adds a loop checking all `nb_intr` entries in `efds[]`, logging a warning for any entry >= 0
- In `rte_intr_efd_disable()`:
  - After closing eventfds (VFIO_MSIX case), adds a loop to reset all `nb_efd` entries to -1 unless type is VDEV
- Updates `rte_intr_efds_index_set()` Doxygen to note that entries holding valid FDs at free time are reported as leaks, and callers managing the array must reset entries to -1 when FDs are closed

**Findings:**

### Errors

None.

### Warnings

None.

---

## Series-Wide Observations

**Correctness:**
- All patches address real resource leaks or spurious leak warnings.
- The interrupt handle ownership model is clarified: handles are no longer cloned by the interrupt subsystem; callers own the handle and its FDs.
- Error paths are correctly fixed to release resources on partial failures.
- The series systematically eliminates FD leaks in alarm, uevent, PCI VFIO, and several drivers.

**Style and Process:**
- All patches are concise and well-targeted.
- Release notes are correctly updated for the API signature change in patch 3.
- Commit messages are clear and reference the correct Fixes: tags and Cc: stable where appropriate.

**API:**
- Patch 3 changes `rte_intr_unregister_callback_fn` to take `const struct rte_intr_handle *`, which is an API change but correctly documented.

---

## Final Assessment

No correctness bugs, style violations, or process issues identified. All patches are clean.

Review-Result: CLEAN