|WARNING| [RFC,v3,5/5] interrupts: warn on leaked file descriptors

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-07

# DPDK Patch Review

## Summary

This 5-patch series addresses file descriptor handling issues in the DPDK interrupt subsystem. The patches include bug fixes, API improvements, and additional safety checks.

---

## Patch 1/5: vdpa/mlx5: fix check on err interrupt FD

### Errors

None.

### Warnings

None.

### Info

The patch correctly fixes FD validation: 0 is a valid file descriptor (stdin), so checking `!rte_intr_fd_get()` is wrong. Using -1 as the invalid sentinel and `< 0` check is correct.

The fix is applied in both the error path (`mlx5_vdpa_err_event_setup`) and the unset function (`mlx5_vdpa_err_event_unset`), and adds the missing reset to -1 after cleanup.

---

## Patch 2/5: eal/freebsd: fix a FD leak in the alarm subsystem

### Errors

None.

### Warnings

None.

### Info

The patch correctly addresses a file descriptor leak in the FreeBSD alarm subsystem. The `fd` variable is now initialized to -1, and the cleanup path properly closes it on error. The successful path already had `rte_intr_fd_set(intr_handle, fd)`, so the FD ownership transfers to `intr_handle` and is cleaned up via `rte_intr_instance_free()` in `rte_eal_alarm_cleanup()`.

The error path now correctly closes the FD before freeing the interrupt handle instance, preventing the leak identified in the commit message.

---

## Patch 3/5: interrupts: mark file descriptors invalid on allocation

### Errors

None.

### Warnings

- **Missing `Cc: stable at dpdk.org`**: This is an API behavior change that affects all interrupt handle users. While it's documented in the release notes as an API change rather than a bug fix, the improved initialization behavior (avoiding confusion with FD 0) suggests this could be backported to stable releases. The patch author should consider whether this qualifies for stable backport.

### Info

This patch improves the interrupt handle allocation by initializing all file descriptors to -1 instead of 0, following Unix conventions. The change is correctly:

1. Documented in `rte_interrupts.h` API documentation
2. Noted in release notes as an API change
3. Tested in the unit test (`test_interrupts.c`)
4. Applied consistently to `fd`, `dev_fd`, and all `efds` array elements
5. Extended to cover `rte_intr_event_list_update()` for newly allocated efds

The FreeBSD PCI implementation update (`drivers/bus/pci/bsd/pci.c`) correctly changes the check from `if (rte_intr_fd_get(...))` to `if (rte_intr_fd_get(...) >= 0)`.

The test suite hack for UIO is acceptable as a workaround for the existing test design limitation (UIO doesn't actually require a device FD).

Removing now-redundant initialization calls across multiple drivers is correct - these were setting FDs to -1 immediately after allocation, which is now done by the allocator itself.

---

## Patch 4/5: interrupts: close interrupt FDs

### Errors

None.

### Warnings

None.

### Info

This patch adds convenience wrappers `rte_intr_fd_close()` and `rte_intr_dev_fd_close()` to encapsulate the common pattern:

```c
if (rte_intr_fd_get(handle) >= 0) {
    close(rte_intr_fd_get(handle));
    rte_intr_fd_set(handle, -1);
}
```

The implementation is correct:
- `rte_intr_fd_close()` is marked experimental and exported with `RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_intr_fd_close, 26.11)`
- `rte_intr_dev_fd_close()` is marked internal and exported with `RTE_EXPORT_INTERNAL_SYMBOL(rte_intr_dev_fd_close)`
- Both functions check `>= 0` before closing
- Both reset the FD to -1 after closing
- Calling them on a handle with FD already -1 is a safe no-op
- Comprehensive unit test coverage added

The conversions across multiple drivers correctly replace the old pattern with the new wrappers, simplifying code and ensuring consistent error handling.

The addition of `plt_intr_fd_close` and `plt_intr_dev_fd_close` wrappers in `drivers/common/cnxk/roc_platform.h` is appropriate for the cnxk driver's platform abstraction layer.

---

## Patch 5/5: interrupts: warn on leaked file descriptors

### Errors

None.

### Warnings

- **Potential for false positives acknowledged**: The commit message states "This will likely raise false positives" and the warnings are at INFO/NOTICE level rather than ERROR. This is an acceptable approach for gradual hardening - it makes leaks visible without breaking existing code.

### Info

This patch adds defensive checks in `rte_intr_instance_free()` to warn about file descriptors that are still valid (>= 0) when the interrupt handle is freed. This helps catch FD leaks at development time.

The patch correctly adds comments throughout drivers explaining when FDs are "owned by" another subsystem (VFIO, ibverbs, vhost, gpio, etc.) and only clearing the reference rather than closing the FD. These are the cases that would trigger the new warning but are intentional:

**Correct ownership transfer patterns identified:**
- `drivers/bus/cdx/cdx_vfio.c`: "vfio_dev_fd is owned by VFIO"
- `drivers/bus/pci/linux/pci_vfio.c`: "vfio_dev_fd is managed by VFIO layer"
- `drivers/common/mlx5/linux/mlx5_common_os.c`: "fd is not owned by the driver"
- `drivers/net/mana/mana.c`: "fd is owned by ibverbs"
- `drivers/net/mlx4/mlx4_intr.c`: "fd is owned by ibverbs"
- `drivers/net/sxe2/sxe2_irq.c`: "fd is not owned by the driver"
- `drivers/net/tap/rte_eth_tap.c`: "fd is not owned by the driver"
- `drivers/net/virtio/virtio_user/virtio_user_dev.c`: "fd is not owned by the driver"
- `drivers/raw/cnxk_gpio/cnxk_gpio.c`: "fd is owned by gpio"
- `drivers/vdpa/mlx5/mlx5_vdpa_event.c`: "fd is owned by err_chnl"
- `drivers/vdpa/mlx5/mlx5_vdpa_virtq.c`: "fd is owned by vhost"

The unit test change in `app/test/test_interrupts.c` adds explicit resets to -1 before freeing interrupt handles, which is correct - these test handles don't have "external ownership" and should clean up their FDs properly.

The warning implementation checks both `fd` and `dev_fd`, logging at `EAL_LOG(NOTICE, ...)` level. This is appropriate - visible enough to catch bugs but not an outright error that would break applications.

---

## Overall Series Assessment

**This is a high-quality patch series that improves FD handling safety in DPDK.**

### Strengths:
1. Correctness bugs fixed (Patch 1: wrong FD validation; Patch 2: FD leak)
2. Consistent API improvement (Patch 3: -1 initialization convention)
3. Code simplification via wrappers (Patch 4: close helpers)
4. Defensive programming (Patch 5: leak detection)
5. Comprehensive test coverage added
6. Clear documentation of FD ownership patterns
7. Proper use of `RTE_EXPORT_*` macros with correct versioning
8. Release notes appropriately updated

### Minor notes:
- Patch 3 could potentially be backported to stable (not tagged `Cc: stable at dpdk.org`)
- Patch 5's warnings may produce false positives but this is acknowledged and acceptable

### Recommendation:
**Acceptable for merge** after author considers whether Patch 3 should target stable branches.