|WARNING| [v3,8/8] interrupts: warn on leaked event FDs

[dpdklab at iol.unh.edu]

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

_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

**Use-after-free potential on error path (line 124)**

```c
if (src->nb_intr > intr_handle->nb_intr &&
		rte_intr_event_list_update(intr_handle, src->nb_intr) != 0) {
	rte_intr_instance_free(intr_handle);
	return NULL;
}
```

After `rte_intr_event_list_update()` fails, `intr_handle->efds` and `intr_handle->elist` may have been partially reallocated. The subsequent `rte_intr_instance_free()` will attempt to free the new arrays, but if the realloc succeeded for one array and failed for the other, the function will free a valid new array and then attempt to access the old pointer that was already freed by realloc. This is a use-after-free.

**Suggested fix:** Audit `rte_intr_event_list_update()` to ensure it either succeeds completely or leaves the handle unchanged. If it cannot be made atomic, document the post-failure state and adjust the cleanup here.

### Warnings

**Missing validation of `rte_intr_event_list_update()` return value consistency**

The patch assumes `rte_intr_event_list_update()` either succeeds fully or fails fully, but does not verify this. If the function can leave the handle in a partially updated state (one array grown, the other not), the subsequent `memcpy()` calls on lines 150-151 will copy `src->nb_intr` elements into arrays that may not have been resized, causing buffer overflows.

**Suggested approach:** Add a comment or assertion documenting the expected behavior of `rte_intr_event_list_update()`, or verify both arrays match `src->nb_intr` after the call.

---

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

### Errors

None.

### Warnings

None.

### Info

This patch correctly fixes the memcpy byte count. The original code copied only `src->nb_intr` bytes instead of `src->nb_intr * sizeof(element)`, which is a correctness bug that the patch addresses.

---

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

### Errors

None.

### Warnings

None.

### Info

The logic change from checking `max_intr > nb_efd` to checking `type == RTE_INTR_HANDLE_VFIO_MSIX` is correct. The old heuristic was fragile and caused double-close bugs in virtio-user. The new approach explicitly identifies the only case where EAL owns the event fds.

---

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

### Errors

**Missing lock around state mutation (lines 1575-1605)**

The function modifies shared state in `intr_handle` without holding any lock. If two threads call `rte_intr_efd_enable()` concurrently on the same handle, or if one thread calls enable while another reads the handle, the EBUSY check on line 1580 is a TOCTOU race: thread A checks `nb_efd == 0`, thread B also checks, both proceed, both allocate eventfds, and one overwrites the other's work.

The existing check `if (rte_intr_nb_efd_get(intr_handle) != 0) { return -EBUSY; }` does not prevent the race because the check and the subsequent modifications are not atomic.

**Suggested fix:** Document that the caller must serialize access to the interrupt handle (as is likely already the case for ethdev control-path operations), or add a lock if concurrent enable calls are expected.

### Warnings

None.

### Info

The error path cleanup (lines 1629-1634) is correct: it closes only the eventfds that were successfully created and recorded in `nb_efd`, and resets the count to zero.

---

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

### Errors

None.

### Warnings

None.

### Info

This patch correctly clears all FD references in the interrupt handle copy before freeing it. The source owns none of the descriptors, so clearing them prevents false-positive leak warnings.

---

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

### Errors

None.

### Warnings

None.

### Info

This is the FreeBSD equivalent of patch 5/8. The pattern is identical: clear FD references in the handle copy before freeing it.

---

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

### Errors

None.

### Warnings

None.

### Info

The fix correctly clears the device FD reference after retrieving it. The FD is managed by the VFIO layer, so the handle must not hold a stale reference that would be reported as a leak.

---

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

### Errors

None.

### Warnings

None.

### Info

This patch extends the FD leak warning to the event FD array. The change to `rte_intr_efd_disable()` (resetting all entries to -1 after closing the owned fds) ensures that a properly disabled handle does not trigger false-positive warnings.

---

## Series-Level Observations

1. **Patch ordering:** The series is well-ordered. Patches 1-4 fix bugs in the interrupt handle duplication and enable/disable paths. Patches 5-7 fix false-positive leak warnings introduced by an earlier commit. Patch 8 extends the leak detection to event FDs, building on the fixes in patches 1-7.

2. **Compilation:** Each patch compiles independently (assuming the DPDK tree is otherwise clean). No forward dependencies detected.

3. **Testing:** The patches fix bugs in error paths and resource cleanup. Functional tests exercising these paths (allocation failures, repeated enable/disable, device close) would be valuable but are not included in this series.

---

Review-Result: ERROR