|SUCCESS| [v4,8/8] interrupts: warn on leaked event FDs

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/8: interrupts: fix size of duplicated instance

### Errors

**Missing error check on `rte_intr_event_list_update()` return**

Line 123: `if (src->nb_intr > intr_handle->nb_intr && rte_intr_event_list_update(intr_handle, src->nb_intr) != 0)`

While the patch adds cleanup on failure (`rte_intr_instance_free(intr_handle)`), it assumes `rte_intr_event_list_update()` will only fail due to allocation failures. If the function can fail for other reasons (e.g., invalid parameters), those errors are not distinguished. However, examining the context, the cleanup path appears correct for the allocation failure case.

**Potential resource leak on partial copy failure**

Lines 131-132:
```c
memcpy(intr_handle->efds, src->efds, src->nb_intr * sizeof(*src->efds));
memcpy(intr_handle->elist, src->elist, src->nb_intr * sizeof(*src->elist));
```

These `memcpy()` operations copy `src->nb_intr` entries into arrays that have been sized to hold `src->nb_intr` entries (after the `rte_intr_event_list_update()` call). However, if `src->nb_intr` exceeds the destination array capacity due to a logic error, this would be a buffer overflow. The code assumes `rte_intr_event_list_update()` guarantees the arrays are large enough, which appears correct but should be verified.

The more pressing concern: if either `memcpy()` were to fail (though `memcpy()` itself cannot fail in the traditional sense), the partially initialized `intr_handle` would still contain references to FDs owned by `src`. This is addressed in later patches but worth noting here.

## Patch 2/8: interrupts: fix event arrays copy on instance duplication

### Info

**Correct fix for array copy**

Lines 134-135:
```c
memcpy(intr_handle->efds, src->efds, src->nb_intr * sizeof(*src->efds));
memcpy(intr_handle->elist, src->elist, src->nb_intr * sizeof(*src->elist));
```

This patch correctly fixes the byte count in the `memcpy()` calls. The original code used `src->nb_intr` as the byte count, which would only copy a fraction of the array elements. The fix multiplies by `sizeof(*src->efds)` and `sizeof(*src->elist)`, which is the correct approach.

No issues found.

## Patch 3/8: eal/linux: fix closing of event FDs not owned by EAL

### Info

**Correct type-based ownership check**

Lines 1626-1627:
```c
if (rte_intr_type_get(intr_handle) == RTE_INTR_HANDLE_VFIO_MSIX) {
    for (i = 0; i < (uint32_t)rte_intr_nb_efd_get(intr_handle); i++)
```

The patch correctly identifies that the original condition (`max_intr > nb_efd`) was an indirect side effect of how `rte_intr_efd_enable()` fills the handle for VFIO MSI-X, not a proper ownership indicator. Checking the interrupt handle type (`RTE_INTR_HANDLE_VFIO_MSIX`) directly is the right approach to identify which eventfds EAL created and owns.

No issues found.

## Patch 4/8: eal/linux: fix eventfd leaks when enabling interrupts

### Errors

**Error path may not close all eventfds on second failure**

Lines 1630-1633 (error path):
```c
error:
	for (int i = 0; i < rte_intr_nb_efd_get(intr_handle); i++) {
		close(rte_intr_efds_index_get(intr_handle, i));
		rte_intr_efds_index_set(intr_handle, i, -1);
```

While this error path closes all eventfds recorded in `nb_efd`, there's a subtle issue: if `rte_intr_efds_index_set()` fails (line 1598) or `rte_intr_nb_efd_set()` fails (line 1598), the just-created eventfd is closed (line 1600) and its entry is cleared (line 1601), then the code jumps to `error`. At that point, `nb_efd` has already been incremented (line 1598), so the error path will attempt to access `efds[i]` for an index where the entry was just cleared. This is not a leak, but the logic is confusing.

Actually, re-reading: line 1598 sets `nb_efd` to `i + 1` AFTER checking if `efds_index_set()` succeeded. If either call fails, `close(fd)` is called immediately and the entry is cleared, but `nb_efd` is NOT incremented (the increment happens via the `rte_intr_nb_efd_set(intr_handle, i + 1)` call). Then `goto error` jumps to the cleanup, which loops up to `nb_efd` (which is still the old value). So if `rte_intr_efds_index_set()` fails on index 3, `nb_efd` is still 3 (not 4), and the error path will close indices 0, 1, 2, but not 3 (which was already closed on line 1600). This is correct.

However, there's a different issue: if `rte_intr_max_intr_set()` fails (line 1606), the error path is reached with `nb_efd` set to `n`, and all eventfds created are closed correctly. But the check on line 1581 (`if (rte_intr_nb_efd_get(intr_handle) != 0)`) means that if `rte_intr_efd_enable()` is called twice, the second call returns `-EBUSY`. This is correct behavior and prevents the leak.

On further analysis, the error handling appears correct.

### Info

**EBUSY on repeated enable calls**

Lines 1580-1583:
```c
if (rte_intr_nb_efd_get(intr_handle) != 0) {
    EAL_LOG(ERR, "Interrupt eventfds are already enabled");
    return -EBUSY;
}
```

This correctly prevents repeated calls to `rte_intr_efd_enable()` from leaking eventfds by checking if eventfds are already enabled before proceeding. Good defensive programming.

No issues found.

## Patch 5/8: eal/linux: fix leaked FD warnings for interrupt sources

### Info

**Correct FD reference clearing in source copy**

Lines 104-112 (new function `eal_intr_source_free()`):
```c
static void
eal_intr_source_free(struct rte_intr_source *src)
{
	struct rte_intr_handle *intr_handle = src->intr_handle;
	int i;

	rte_intr_fd_set(intr_handle, -1);
	rte_intr_dev_fd_set(intr_handle, -1);
	for (i = 0; i < rte_intr_nb_intr_get(intr_handle); i++)
		rte_intr_efds_index_set(intr_handle, i, -1);
	rte_intr_instance_free(intr_handle);
	free(src);
}
```

This correctly clears FD references in the interrupt handle copy before freeing it. The source owns a copy of the handle but not the FDs themselves, so clearing the references prevents false leak warnings.

No issues found.

## Patch 6/8: eal/freebsd: fix interrupt source handle leak

### Info

**Same fix as Patch 5, for FreeBSD**

Lines 66-84:
```c
static void
eal_intr_source_free(struct rte_intr_source *src)
{
	struct rte_intr_handle *intr_handle = src->intr_handle;
	int i;

	rte_intr_fd_set(intr_handle, -1);
	rte_intr_dev_fd_set(intr_handle, -1);
	for (i = 0; i < rte_intr_nb_intr_get(intr_handle); i++)
		rte_intr_efds_index_set(intr_handle, i, -1);
	rte_intr_instance_free(intr_handle);
	free(src);
}
```

Identical fix to Patch 5, applied to FreeBSD's interrupt handling code.

No issues found.

## Patch 7/8: bus/pci: fix leaked device FD warning on VFIO unmap

### Info

**Correct device FD reference clearing**

Lines 1106-1107 and 1147-1148:
```c
/* vfio_dev_fd is managed by VFIO layer, only clear reference here. */
rte_intr_dev_fd_set(dev->intr_handle, -1);
```

This correctly clears the device FD reference in the interrupt handle after retrieving it for release. The VFIO layer owns the device FD, so the interrupt handle should not report it as leaked when freed.

No issues found.

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

### Info

**Event FD leak detection**

Lines 204-208:
```c
for (int i = 0; i < intr_handle->nb_intr; i++) {
	if (intr_handle->efds[i] >= 0)
		EAL_LOG(NOTICE, "Some interrupt handle is leaking an event FD: %d (index %d)",
			intr_handle->efds[i], i);
}
```

This extends the leak warning to the event FD array, which is consistent with the existing warnings for the handle FD and device FD.

**Event FD entries reset after disable**

Lines 1670-1671:
```c
for (i = 0; i < (uint32_t)rte_intr_nb_efd_get(intr_handle); i++)
	rte_intr_efds_index_set(intr_handle, i, -1);
```

This correctly resets all event FD entries to -1 after closing them (or in the non-MSI-X case, after they are no longer referenced). This prevents false leak warnings for properly disabled handles.

No issues found.

---

## Summary

The patch series addresses several resource management issues in the DPDK interrupt handling code:

1. Fixes incorrect array sizing when duplicating interrupt instances
2. Fixes incorrect byte counts in array copies
3. Fixes incorrect FD ownership checks leading to double-close
4. Fixes eventfd leaks on error paths during enable
5. Fixes false leak warnings for interrupt source handle copies (Linux)
6. Fixes false leak warnings for interrupt source handle copies (FreeBSD)
7. Fixes false leak warnings for VFIO device FD
8. Adds leak detection for event FDs

All patches correctly address their stated issues. The error handling in Patch 4 is subtle but appears correct after careful analysis.

Review-Result: CLEAN