|WARNING| [RFC,v4,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/169197

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

**Error: Resource leak on error path**

In `mlx5_vdpa_err_event_setup()`, when `rte_intr_callback_register()` fails at line 453, the function returns after setting the fd to -1 but never closes the file descriptor that was opened earlier. The `priv->err_chnl` event channel and its associated fd remain allocated.

```c
if (ret != 0) {
	rte_intr_fd_set(priv->err_intr_handle, -1);
	DRV_LOG(ERR, "Failed to register error interrupt for device %d.",
		priv->vid);
	rte_errno = -ret;
	return -1;  /* ERROR: err_chnl and its fd leaked */
}
```

**Suggested fix:** Add cleanup before returning:
```c
if (ret != 0) {
	rte_intr_fd_set(priv->err_intr_handle, -1);
	if (priv->err_chnl) {
		mlx5_glue->devx_destroy_event_channel(priv->err_chnl);
		priv->err_chnl = NULL;
	}
	DRV_LOG(ERR, "Failed to register error interrupt for device %d.",
		priv->vid);
	rte_errno = -ret;
	return -1;
}
```

---

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

### Warnings

**Warning: Missing error propagation**

In `rte_eal_alarm_cleanup()` at line 57, if `rte_intr_callback_unregister_sync()` returns an error (ret < 0), the function silently continues without propagating the error to the caller. This could mask cleanup failures.

```c
int ret = rte_intr_callback_unregister_sync(intr_handle,
		eal_alarm_callback, (void *)-1);
if (ret >= 0) {
	rte_intr_fd_close(intr_handle);
	rte_intr_instance_free(intr_handle);
	intr_handle = NULL;
}
/* No indication of failure if ret < 0 */
```

Consider logging a warning or returning the error status.

---

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

### Errors

None identified.

### Warnings

**Warning: Unit test hack comment is misleading**

At lines 95-96 and 116-117 in `app/test/test_interrupts.c`, the comments say "HACK: UIO type does not require a device FD, but a valid handle should contain one". This is confusing because:
1. If UIO doesn't require a device FD, why is INT_MAX considered "valid"?
2. The hack suggests the test infrastructure needs fixing rather than working around it.

```c
/* HACK: UIO type does not require a device FD, but a valid handle should contain one */
if (rte_intr_dev_fd_set(test_intr_handle, INT_MAX))
	return -1;
```

Consider either fixing the test to not require this workaround or clarifying why INT_MAX is the correct sentinel value for this test case.

---

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

### Errors

None identified.

### Warnings

**Warning: Inconsistent API marking**

`rte_intr_fd_close()` is marked `__rte_experimental` (line 291) while `rte_intr_dev_fd_close()` is marked `__rte_internal` (line 280). This inconsistency means:
- Applications can call `rte_intr_fd_close()` (experimental, will become stable)
- Only DPDK internal code can call `rte_intr_dev_fd_close()` (internal, never exported)

If both functions serve the same conceptual purpose (closing and resetting an fd), they should have the same visibility. If `dev_fd` is genuinely internal-only, this is correct; otherwise, both should be experimental.

**Suggested action:** Document in the commit message why `rte_intr_dev_fd_close()` is internal-only while `rte_intr_fd_close()` is experimental, or make them both experimental if applications need both.

---

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

### Errors

**Error: Potential false positive warnings on normal usage**

In `rte_intr_instance_free()` (lines 190-194), the function logs NOTICE-level warnings for any fd >= 0. This will produce false positives in legitimate cases where:
1. The driver intentionally clears references but doesn't own the fd (as documented in several places in this patch: "fd is owned by ibverbs, only clear reference here")
2. The fd is still in use and will be closed later via a different path

```c
if (rte_intr_fd_get(intr_handle) >= 0)
	EAL_LOG(NOTICE, "Some interrupt handle is leaking a FD: %d",
		rte_intr_fd_get(intr_handle));
```

This check conflates two distinct cases:
- **Leak**: driver forgot to close an fd it owns
- **Reference**: driver is clearing a reference to an fd owned by another component (ibverbs, VFIO layer, etc.)

**Suggested fix:** Either:
1. Add a flag to `rte_intr_handle` to indicate whether the fd is owned vs referenced, and only warn on owned fds
2. Change the log level to DEBUG and clarify the message: "interrupt handle freed with active fd reference"
3. Document that drivers MUST call `rte_intr_fd_close()` even if they don't own the fd (it will be a no-op after the driver sets fd to -1)

Currently, with this patch applied, every driver that documents "fd is not owned by the driver, only clear reference here" will trigger a false positive warning.

---

## General Observations

### Positive aspects:
- The series addresses real resource leak issues (patches 1-2)
- The new `rte_intr_fd_close()` helpers improve code clarity (patch 4)
- Comprehensive updates across many drivers (patch 4)
- Good unit test coverage (patches 3-4)

### Style compliance:
- All patches follow DPDK C coding style
- Proper use of explicit comparisons (`>= 0`, `!= NULL`)
- Release notes updated appropriately
- Commit messages follow the standard format

### Documentation:
- API changes documented in release notes
- Function-level documentation added for new APIs
- Inline comments explain fd ownership model

---

## Summary

**Must fix (Errors):**
1. Patch 1: Resource leak in `mlx5_vdpa_err_event_setup()` error path
2. Patch 5: False positive warnings for non-owned fds in `rte_intr_instance_free()`

**Should fix (Warnings):**
1. Patch 2: Silent error swallowing in `rte_eal_alarm_cleanup()`
2. Patch 3: Misleading hack comment in unit test
3. Patch 4: Document visibility difference between `fd_close()` and `dev_fd_close()`

The series makes valuable improvements to interrupt handling, but the leak detection in patch 5 needs refinement to avoid false positives, and patch 1 has a real resource leak that must be fixed.