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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Resource leak on FD close failure:**
The patch adds `rte_intr_fd_close(intr_handle)` before `rte_intr_instance_free(intr_handle)`, but only when `rte_intr_callback_unregister_sync()` returns `>= 0`. If the unregister fails (returns `< 0`), the function returns early without closing the FD or freeing the handle, leaking both resources.

```c
int ret = rte_intr_callback_unregister_sync(intr_handle,
					eal_alarm_callback, (void *)-1);
if (ret >= 0) {
	rte_intr_fd_close(intr_handle);
	rte_intr_instance_free(intr_handle);
	intr_handle = NULL;
	handler_registered = 0;
}
/* If ret < 0, function returns here without cleanup */
```

**Suggested fix:**
```c
int ret = rte_intr_callback_unregister_sync(intr_handle,
					eal_alarm_callback, (void *)-1);
if (ret < 0)
	EAL_LOG(ERR, "Failed to unregister alarm callback");
rte_intr_fd_close(intr_handle);
rte_intr_instance_free(intr_handle);
intr_handle = NULL;
handler_registered = 0;
```

---

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

### No Issues

Clean fix using `rte_intr_fd_close()` instead of bare `close()`.

---

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

### Errors

**Missing Cc: stable at dpdk.org:**
This is a correctness bug fix (fixes wrong handle passed to callbacks, prevents pool corruption). The `Fixes:` tag is present but `Cc: stable at dpdk.org` is missing.

### Info

The patch converts interrupt source matching from FD-based to pointer-based. The reasoning is sound: cloning caused the wrong handle to be passed to callbacks when multiple handles shared the same FD. The new approach enforces the existing invariant (one handle per FD) and passes the caller's own handle back.

---

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

### Warnings

**Incomplete secondary process cleanup:**
The patch removes the `rte_intr_dev_fd_set(dev->vfio_req_intr_handle, vfio_dev_fd)` call in `pci_vfio_map_resource_secondary()` because the fd is not used in secondary processes. However, it does not add a corresponding `rte_intr_dev_fd_set(dev->vfio_req_intr_handle, -1)` in the unmap paths (`pci_vfio_unmap_resource_secondary()` or error cleanup). If the `vfio_req_intr_handle` is freed without being reset, a spurious leak warning could occur on the dev_fd, though this is less critical than in the primary process.

**Suggested fix:**
Add `rte_intr_dev_fd_set(dev->vfio_req_intr_handle, -1)` in `pci_vfio_unmap_resource_secondary()` if the `vfio_req_intr_handle` is allocated and later freed.

---

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

### No Issues

Correctly clears FD reference before freeing interrupt handle. The comment "fd is owned by ibverbs" is accurate.

---

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

### Warnings

**Error path in memif_socket_init may double-free:**
When `rte_hash_add_key_data()` fails, the error path now cleans up the listener socket:
```c
if (socket->listener) {
	rte_intr_callback_unregister(socket->intr_handle, ...);
	close(rte_intr_fd_get(socket->intr_handle));
	rte_intr_fd_set(socket->intr_handle, -1);
	rte_intr_instance_free(socket->intr_handle);
}
rte_free(socket);
```

However, this code path is reached only when `socket` was just created by `memif_socket_create()`. If `memif_socket_create()` succeeded, the socket is already fully initialized. But the function does not check whether the socket was newly created or retrieved from the hash. If the socket came from the hash (the `socket = tmp_socket` branch earlier in the function), the cleanup here would unregister and free a socket still in use by other devices.

On closer inspection: the error path is only reachable when `tmp_socket == NULL` (new socket creation), because when `tmp_socket != NULL`, the function either succeeds or fails before calling `memif_socket_create()`. So this is actually safe, but the logic is fragile.

**Suggested improvement (optional):**
Add a comment or assertion that this cleanup only applies to newly created sockets, or restructure to make the invariant clearer.

---

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

### No Issues

Correctly uses `rte_intr_fd_close()` where the FD is owned by the handle, and sets FD to `-1` with a comment where the FD is closed separately.

---

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

### No Issues

Correctly clears FD reference on error. Comment is accurate.

---

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

### No Issues

Correctly clears FD reference before freeing. Comment explains that the FD was already closed by `rte_intr_efd_disable()`.

---

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

### Warnings

**Type check may not cover all cases:**
The fix changes the condition in `rte_intr_efd_disable()` from `max_intr > nb_efd` (which accidentally matched VFIO MSI-X due to the extra slot for misc interrupt) to an explicit type check:
```c
if (rte_intr_type_get(intr_handle) == RTE_INTR_HANDLE_VFIO_MSIX) {
```

This is correct for known types, but if a new interrupt type in the future creates eventfds in `rte_intr_efd_enable()`, it would not be covered by the type check and those fds would leak. The original heuristic was fragile but somewhat forward-compatible.

**Suggested improvement (optional):**
Add a comment in `rte_intr_efd_enable()` documenting that any new type creating eventfds there must be added to the type check in `rte_intr_efd_disable()`, or consider a flag in the handle to track ownership.

---

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

### No Issues

Correctly rejects repeated enable with `EBUSY` and updates `nb_efd` incrementally so the error path can clean up partial allocations.

---

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

### No Issues

Extends leak warnings to event FDs. The logic is consistent with the existing FD and dev_fd leak checks.

---

## Summary

### Errors (1)

**Patch 1/12:**
- Resource leak if `rte_intr_callback_unregister_sync()` fails: FD and handle not freed on error path.

**Patch 3/12:**
- Missing `Cc: stable at dpdk.org` for a correctness bug fix.

### Warnings (3)

**Patch 4/12:**
- Secondary process unmap does not reset `vfio_req_intr_handle` dev_fd, may cause spurious leak warning.

**Patch 6/12:**
- Error path in `memif_socket_init()` could theoretically double-free if invariants are violated (currently safe but fragile).

**Patch 10/12:**
- Type-based check in `rte_intr_efd_disable()` may not cover future types that create eventfds.

---

Review-Result: ERROR