|SUCCESS| [v2, 3/3] eal/linux: fix eventfd leaks when enabling interrupts

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None identified.

### Warnings

None identified.

### Info

The fix correctly identifies that the ownership check `max_intr > nb_efd` was indirect and fragile. Switching to an explicit type check (`RTE_INTR_HANDLE_VFIO_MSIX`) is clearer and prevents EAL from closing file descriptors it does not own. The logic is sound: only eventfds created by `rte_intr_efd_enable()` for VFIO MSI-X should be closed by `rte_intr_efd_disable()`.

---

## Patch 2/3: interrupts: warn on leaked event FDs

### Errors

None identified.

### Warnings

None identified.

### Info

The added leak detection for event FDs in `rte_intr_instance_free()` is consistent with the existing checks for handle FD and device FD. The loop correctly iterates over `intr_handle->nb_intr` and logs any entry still holding a valid file descriptor (>= 0).

The clearing of event FD entries in `rte_intr_efd_disable()` is appropriate: after closing the owned descriptors, all entries are reset to -1 so that a properly cleaned-up handle does not trigger false leak warnings.

The documentation updates in `rte_interrupts.h` correctly inform callers that they are responsible for resetting entries to -1 when managing the array themselves.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

**Correctness of the fix:**

The original code had two issues:
1. **Partial allocation leak**: If `eventfd()` or `rte_intr_efds_index_set()` failed partway through, already-created descriptors were left open because `nb_efd` was only updated after all allocations succeeded.
2. **Re-enable leak**: Calling `rte_intr_efd_enable()` again without first calling `rte_intr_efd_disable()` would overwrite existing eventfd entries without closing them.

The fix addresses both:
- **Re-enable guard**: The new check rejects repeated enable calls with `-EBUSY` if `nb_efd != 0`, preventing overwriting of existing descriptors.
- **Incremental count update**: Each eventfd is recorded in the count as it is created (`rte_intr_nb_efd_set(intr_handle, i + 1)`), so the error path knows exactly which descriptors exist and can close them.
- **Error path cleanup**: On failure, the loop closes all eventfds recorded in the count, clears their entries to -1, and resets `nb_efd` to 0, leaving the handle in a clean state.

**Verification of error path logic:**

The error path iterates `for (int i = 0; i < rte_intr_nb_efd_get(intr_handle); i++)` and closes each descriptor. This is correct because `nb_efd` now reflects exactly how many descriptors were successfully created before the failure.

The `close()` call is followed by `rte_intr_efds_index_set(intr_handle, i, -1)` to clear the entry, consistent with the leak detection mechanism added in Patch 2/3.

Finally, `rte_intr_nb_efd_set(intr_handle, 0)` resets the count so that a subsequent `rte_intr_efd_enable()` call (after fixing the underlying issue) can proceed without hitting the `-EBUSY` guard.

**C99 loop variable declaration:**

The error path uses `for (int i = 0; ...)`, which is C99 declaration-at-point-of-use style. This is acceptable per the guidelines ("Variables may be declared either at the start of the block, or at point of first use (C99 style)").

The main loop in the VFIO_MSIX branch already declared `uint32_t i;` at the start of the block, so the error path's `int i` is in a different scope and does not conflict.

---

## Summary

All three patches are correct. They fix real bugs:
- Patch 1/3 prevents double-close of file descriptors not owned by EAL.
- Patch 2/3 adds leak detection to catch drivers that fail to clean up event FDs.
- Patch 3/3 fixes resource leaks in the eventfd enable path when allocation fails or enable is called twice.

No style violations, no missing documentation, no ABI concerns. The Fixes tags and `Cc: stable at dpdk.org` are appropriate for backporting.

Review-Result: CLEAN