|WARNING| [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/170500

_AI Code Review_

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

# DPDK Patch Review

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

**No issues found.**

---

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

### Errors

**Error path resource leak (file descriptor):**
The patch fixes one leak but introduces another in the error path of `rte_eal_alarm_init()`. When `kqueue(2)` succeeds but `rte_intr_callback_register()` fails, the `fd` is not closed before the function returns.

```c
/* Current code after patch: */
int
rte_eal_alarm_init(void)
{
	int fd = -1;

	intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
	if (intr_handle == NULL) {
		EAL_LOG(ERR, "Fail to allocate intr_handle");
		goto error;
	}

	if (rte_intr_type_set(intr_handle, RTE_INTR_HANDLE_ALARM))
		goto error;

	/* ... */
	fd = kqueue();
	if (fd < 0) {
		/* ... */
		goto error;
	}
	/* ... */
	if (rte_intr_callback_register(intr_handle, eal_alarm_callback,
				       (void *)(uintptr_t)-1) < 0) {
		EAL_LOG(ERR, "Fail to register alarm callback");
		goto error;  /* BUG: fd not closed */
	}

	return 0;
error:
	if (fd >= 0)
		close(fd);
	rte_intr_instance_free(intr_handle);
	return -1;
}
```

**Suggested fix:** The error label already handles closing the `fd` if it is >= 0. However, `rte_intr_fd_set()` must succeed before the callback registration so that the error path can find and close the fd. Move the `rte_intr_fd_set()` call before `rte_intr_callback_register()`.

```c
/* After fd = kqueue() and before rte_intr_callback_register(): */
if (rte_intr_fd_set(intr_handle, fd))
	goto error;

if (rte_intr_callback_register(intr_handle, eal_alarm_callback,
			       (void *)(uintptr_t)-1) < 0) {
	EAL_LOG(ERR, "Fail to register alarm callback");
	goto error;
}
```

Note: The current patch has `rte_intr_fd_set()` *after* callback registration, so if registration fails, the fd is leaked because the error label checks `rte_intr_instance_free()` but the fd was never associated with the handle.

Actually, re-reading the patch more carefully: the error label closes `fd` directly using the local variable `fd`, not by querying the interrupt handle. So the leak is covered. My mistake. **Withdraw this error.**

Wait, let me re-check. The error path is:
```c
error:
	if (fd >= 0)
		close(fd);
	rte_intr_instance_free(intr_handle);
	return -1;
```
This looks correct: if `fd` was opened but callback registration failed, the error label closes it. The `rte_intr_fd_set()` happens after successful registration, so on the success path the fd is transferred to the interrupt handle, and on the error path the local `fd` is closed. This is correct.

**No error here.** (False alarm on my part.)

---

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

### Errors

**Missing release notes entry for internal API change:**
The patch updates `rte_intr_instance_alloc()` to initialize file descriptors to -1 instead of 0. This is a behavioral change to a public API function (though the impact is clarifications rather than a break). The release notes entry is present and correct. **No issue.**

**Unit test uses internal API without allowing it:**
The patch adds `#define ALLOW_INTERNAL_API` to the unit test, which is correct for code that needs to access internal interrupt handle fields for testing purposes. **No issue.**

**FreeBSD PCI code conditional check:**
Changed from `if (rte_intr_fd_get(dev->intr_handle))` to `if (rte_intr_fd_get(dev->intr_handle) >= 0)`. This is correct and matches the documented FD validity convention. **No issue.**

### Warnings

**Unit test "HACK" comments:**
The patch adds two instances of:
```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;
```
Using `INT_MAX` as a fake valid device FD is a workaround to satisfy handle validity checks in code that expects both `fd` and `dev_fd` to be valid. While the comment acknowledges this is a hack, a cleaner approach would be to adjust the validity checks or test infrastructure to handle UIO's unusual property of not needing a device FD. Consider refactoring the test to avoid this if feasible.

---

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

**No issues found.** The new helpers `rte_intr_fd_close()` and `rte_intr_dev_fd_close()` encapsulate a common pattern correctly, and conversions throughout the codebase are appropriate.

---

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

### Errors

**None.** The warning on FD leaks is a diagnostic aid, not a functional change. The added `rte_intr_fd_set(..., -1)` calls in various drivers correctly document that those FDs are owned externally (ibverbs, vhost, etc.) and only the reference is being cleared, not the FD closed.

### Warnings

**False positive potential:**
The warnings in `rte_intr_instance_free()` will trigger whenever an interrupt handle is freed without first closing or clearing its FDs. While this is the intended behavior to catch leaks, some legitimate use cases might intentionally not close the FD before freeing the handle (e.g., if the FD is still in use elsewhere). The log level is `NOTICE`, which is appropriate for a diagnostic that might have false positives. Developers will need to audit their code if these warnings appear. **This is acceptable** as a debugging aid, but worth noting.

---

## Summary

**Errors:** None.

**Warnings:**
- Patch 3: Unit test uses `INT_MAX` as a fake device FD (acknowledged as a HACK in comments).
- Patch 5: FD leak warnings may produce false positives in code that intentionally retains FDs after freeing the interrupt handle.

The patch series correctly addresses FD validation and leak detection, with thoughtful handling of ownership semantics. The changes improve code robustness and debugging capability.