|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

**Error - Buffer overflow via undersized memcpy**

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

These `memcpy` calls use `src->nb_intr` as the byte count when copying arrays of structures.
This copies only the first `src->nb_intr` bytes of each array instead of all elements.

If `src->nb_intr` is, say, 4, and `sizeof(*src->efds)` is 4 bytes (int),
then only 4 bytes (1 element) are copied when 4 elements should be.

The subsequent patch (2/8) fixes this exact issue, which confirms this is a real bug.
Even though the fix follows immediately, the series must compile and be correct at each commit
for `git bisect`. Patch 1 introduces a buffer overflow that patch 2 then corrects.

**Recommended fix:** Either squash patches 1 and 2, or correct the `memcpy` byte counts in patch 1
to `src->nb_intr * sizeof(*src->efds)` and `src->nb_intr * sizeof(*src->elist)`.

---

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

### Summary

This patch corrects the `memcpy` byte counts introduced by the previous patch.
The fix itself is correct.

The issue is that patch 1 introduced a buffer overflow (undersized copy),
and patch 2 fixes it. For bisectability, the bug should not be introduced at all,
or both patches should be squashed.

No additional issues in patch 2 itself.

---

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

### Summary

This patch changes `rte_intr_efd_disable()` to close event FDs only for `RTE_INTR_HANDLE_VFIO_MSIX`,
since those are the only ones EAL owns. Other interrupt types (VDEV, EXT) have event FDs
owned by the driver, which should not be closed by EAL.

The logic is correct. The comment added is clear and justified.

No issues found.

---

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

### Errors

**Error - Variable `i` declared outside loop then used in error cleanup loop without initialization**

Line 1630:
```c
for (int i = 0; i < rte_intr_nb_efd_get(intr_handle); i++) {
```

In the error cleanup path at line 1630, a new loop variable `int i` is declared and initialized to 0.
However, earlier in the function at line 1578, another variable `i` is declared at function scope
as `uint32_t i;` (without initialization), then used in the main MSI-X setup loop starting at line 1585.

This is not a bug per se -- the `int i` in the error path is a separate C99 loop-scoped variable
that shadows the outer `uint32_t i`. However, it is potentially confusing that both exist.

Actually, re-reading: the outer `uint32_t i;` at line 1578 is used in the loop at line 1585,
and then the error path declares a fresh `int i` at line 1630. This is acceptable C99 style
and not an error. Disregard this note.

**No errors found in patch 4.**

The logic is correct:
- Rejects repeated enable calls with `EBUSY`.
- Updates `nb_efd` as each eventfd is created so the error path can clean up partial allocations.
- On error, closes all created eventfds, resets their entries to -1, and sets `nb_efd` back to 0.

---

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

### Summary

This patch adds `eal_intr_source_free()`, which clears all FD references in an interrupt source's handle
before freeing it, so the FDs are not reported as leaks. The interrupt source holds a copy of the handle
but does not own any of the FDs (they are owned by whoever registered the source).

The implementation is correct: it sets `fd`, `dev_fd`, and all `efds` entries to -1 before freeing.

No issues found.

---

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

### Summary

This patch is the FreeBSD equivalent of patch 5. It adds the same `eal_intr_source_free()` function
and uses it in place of `free(src)` in three locations where an interrupt source is removed.

The implementation matches the Linux version and is correct.

No issues found.

---

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

### Summary

This patch clears the `dev_fd` reference in the interrupt handle after retrieving it in the PCI VFIO
unmap functions. The device FD is owned by the VFIO layer and closed separately, so the handle
should not report it as leaked when freed.

The fix mirrors the pattern already used for the request notifier handle in the same functions.

No issues found.

---

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

### Summary

This patch extends the leak warning in `rte_intr_instance_free()` to include the event FD array.
It also updates `rte_intr_efd_disable()` to reset all event FD entries to -1 after closing them
(for the MSI-X case) or after the entry was just a reference (for other cases).

The Doxygen update documents the expectation that drivers managing the array themselves
must reset entries to -1 when the FD is closed or no longer referenced.

The implementation is correct and consistent with the existing leak warning pattern.

No issues found.

---

## Cross-Patch Observation

**Patches 1 and 2 should be squashed or reordered.**

Patch 1 introduces a buffer overflow (undersized `memcpy` using element count as byte count).
Patch 2 immediately fixes it by multiplying by `sizeof`.

For `git bisect` correctness, each commit must compile and be correct on its own.
Introducing a bug in patch 1 that is fixed in patch 2 violates this principle.

**Recommendation:** Squash patches 1 and 2 into a single commit that both grows the duplicate
to match the source size AND copies the arrays with the correct byte count.

Alternatively, correct the `memcpy` calls in patch 1 to use the proper size from the start,
making patch 2 unnecessary (though the commit message of patch 1 would then need to mention
the correct byte count).

---

## Summary

- **PATCH 1:** Error -- undersized `memcpy` (buffer overflow)
- **PATCH 2:** Correct fix, but should be squashed with patch 1 for bisectability
- **PATCHES 3-8:** No errors found; all fixes are correct

Review-Result: ERROR