|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

# Review of DPDK Patch Series

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

### Errors

None identified.

### Warnings

None identified.

### Info

The patch correctly changes the FD validation pattern from checking against 0 to checking for values < 0, which is the proper Unix convention for invalid file descriptors. The change is consistent with setting the FD to -1 to indicate an invalid state.

---

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

### Errors

**Resource leak on error path:**
In `rte_eal_alarm_init()`, if `kqueue(2)` succeeds but `rte_intr_type_set()` or `rte_intr_callback_register()` fail, the function jumps to the `error:` label. However, the error path only closes `fd` if `fd >= 0`. The initialization `int fd = -1;` ensures this check works correctly, but the error path should also verify that `rte_intr_fd_set()` succeeded before the failure occurred. If `rte_intr_fd_set()` fails after `kqueue()` but before setting `intr_handle`'s fd field, we have two references to the same FD (`fd` local variable and potentially nowhere else), and only one will be closed.

Actually, reviewing more carefully: after `kqueue()` returns successfully, `fd` holds the kqueue descriptor. If `rte_intr_fd_set(intr_handle, fd)` fails, the error path closes `fd` (the local variable), which is correct. If `rte_intr_fd_set()` succeeds, then the error path must NOT close `fd` via the local variable because ownership has transferred to `intr_handle`. The current code does not handle this correctly: it unconditionally closes `fd` on the error path even after `rte_intr_fd_set()` may have succeeded.

Suggested fix:

```c
int
rte_eal_alarm_init(void)
{
	int fd = -1;

	intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
	if (intr_handle == NULL) {
		RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n");
		goto error;
	}

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

	/* ... */

	fd = kqueue();
	if (fd < 0) {
		RTE_LOG(ERR, EAL, "Cannot create kqueue instance\n");
		goto error;
	}

	if (rte_intr_fd_set(intr_handle, fd))
		goto error;

	/* After rte_intr_fd_set succeeds, intr_handle owns fd; don't close it separately */
	ret = rte_intr_callback_register(intr_handle, eal_alarm_callback, NULL);
	if (ret < 0) {
		RTE_LOG(ERR, EAL, "Failed to register alarm callback.\n");
		/* fd is owned by intr_handle; close via rte_intr_fd_close or similar */
		goto error;
	}

	return 0;
error:
	/* Only close fd if it wasn't transferred to intr_handle */
	if (fd >= 0 && (intr_handle == NULL || rte_intr_fd_get(intr_handle) != fd))
		close(fd);
	rte_intr_instance_free(intr_handle);
	return -1;
}
```

The current patch adds `if (fd >= 0) close(fd);` unconditionally on the error path. This is a **use-after-free risk** if `rte_intr_fd_set()` succeeded but a later function failed: both the error path and `rte_eal_alarm_cleanup()` would attempt to close the same FD, or the FD could be reused by another subsystem and closed incorrectly.

---

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

### Errors

None identified in the code changes themselves.

### Warnings

**Unit test bypass/hack:**
The unit test adds a hack to set `dev_fd` to `INT_MAX` for UIO-type handles because "UIO type does not require a device FD, but a valid handle should contain one." This comment and the hack itself indicate a conceptual mismatch: if UIO truly does not require a device FD, then a valid UIO interrupt handle should be allowed to have `dev_fd == -1`. Forcing `INT_MAX` as a sentinel for testing purposes is a workaround that hides the real issue. The test should either be refactored to not require `dev_fd` for UIO, or the documentation/assertions should clarify when `dev_fd` is required.

Suggested action: Document in the test comments that this is a temporary workaround pending a refactor of the interrupt handle validation logic, or revise the validation to allow `dev_fd == -1` for UIO types.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

The new `rte_intr_fd_close()` and `rte_intr_dev_fd_close()` wrappers correctly encapsulate the close-and-reset pattern. The conversions in drivers replace manual `if (fd >= 0) { close(fd); set_to_-1; }` logic with the wrapper calls, which is a good cleanup. The added comments (`/* fd is owned by ... */`) clarify ownership, which helps prevent future double-free or use-after-free bugs.

The experimental symbol for `rte_intr_fd_close()` and internal symbol for `rte_intr_dev_fd_close()` are appropriate given the API stability policy.

---

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

### Errors

None identified.

### Warnings

**False positive potential:**
The warnings added in `rte_intr_instance_free()` will flag any interrupt handle with a valid FD (`>= 0`) at free time. As noted in the commit message, this will "likely raise false positives." The warnings should clarify that the FD reference may be intentional (e.g., ownership transferred elsewhere, FD not actually leaked), and should not cause test failures or alarm in logs unless a genuine leak is suspected.

Consider rephrasing the log messages to indicate this is a potential issue, not a definite one:

```c
if (rte_intr_fd_get(intr_handle) >= 0)
	EAL_LOG(NOTICE, "Interrupt handle freed with valid FD %d still set. "
		"This may indicate a resource leak if the FD was not intentionally "
		"left open or transferred elsewhere.",
		rte_intr_fd_get(intr_handle));
```

### Info

The added `rte_intr_fd_set(handle, -1)` calls in several drivers (e.g., `cnxk_gpio`, `mana`, `mlx4_intr`, `sxe2_irq`, `tap`, `virtio_user`) with the comment `/* fd is [not] owned by ... */` are good documentation of ownership semantics. This makes it explicit that the driver is clearing a reference without closing the FD because the FD is managed externally.

---

## Summary

### Errors

1. **Patch 2/5: Resource leak and potential use-after-free** in `rte_eal_alarm_init()` error path. After `rte_intr_fd_set(intr_handle, fd)` succeeds, the FD ownership transfers to `intr_handle`. The error path should not close `fd` via the local variable at that point, because `rte_intr_instance_free(intr_handle)` or subsequent cleanup (if any) should handle it. The current code will attempt to close `fd` twice if `rte_intr_fd_set()` succeeded but a later call failed.

### Warnings

1. **Patch 3/5: Unit test hack** (setting `dev_fd = INT_MAX` for UIO handles) works around rather than fixes a conceptual issue. Consider documenting this as a temporary measure or revising the validation logic.

2. **Patch 5/5: False positive potential** in the FD leak warnings. The log messages should clarify that a valid FD at free time is suspicious but may be intentional depending on ownership semantics.

### Info

- Patches 1, 3, 4, and 5 generally follow DPDK coding standards and guidelines.
- The new helper functions `rte_intr_fd_close()` and `rte_intr_dev_fd_close()` are well-designed encapsulations that simplify driver code.
- Release notes and documentation updates are appropriate and complete.
- Ownership comments added in patch 5 are valuable for maintainability.

---

## Detailed Findings by Severity

### Error

**Patch 2/5: `lib/eal/freebsd/eal_alarm.c` - FD double-close risk**

In `rte_eal_alarm_init()`, after `rte_intr_fd_set(intr_handle, fd)` succeeds, the local variable `fd` no longer owns the file descriptor--`intr_handle` does. If a subsequent call fails and the function jumps to `error:`, the line `if (fd >= 0) close(fd);` will close the FD, but `intr_handle` still holds a reference to it. When `rte_intr_instance_free(intr_handle)` is called, or if cleanup is deferred to another function, that same FD may be closed again, or the FD may have been reused by the system and the second close operates on the wrong descriptor.

**Fix:**

Only close `fd` via the local variable if `rte_intr_fd_set()` failed or was never called. Once `rte_intr_fd_set()` succeeds, let `intr_handle` own the FD and do not close it directly:

```c
error:
	/* Only close fd if ownership was never transferred to intr_handle */
	if (fd >= 0) {
		/* Check if intr_handle took ownership */
		if (intr_handle == NULL || rte_intr_fd_get(intr_handle) != fd)
			close(fd);
	}
	rte_intr_instance_free(intr_handle);
	return -1;
```

Alternatively, clear `fd` after a successful `rte_intr_fd_set()`:

```c
if (rte_intr_fd_set(intr_handle, fd))
	goto error;
fd = -1;  /* Ownership transferred, don't close in error path */
```

Then the error path `if (fd >= 0) close(fd);` becomes safe because `fd` is -1 after transfer.

### Warning

**Patch 3/5: `app/test/test_interrupts.c` - UIO test workaround**

The test adds:

```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;
```

This workaround sets `dev_fd` to a nonsensical value (`INT_MAX`) to satisfy some validation that expects `dev_fd` to be valid even for UIO handles. If UIO legitimately does not require a device FD, the validation (or test expectation) should be updated rather than injecting a fake FD.

**Suggestion:**

Document why this is necessary and whether it can be removed in the future, or revise the test to skip `dev_fd` validation for UIO-type handles.

**Patch 5/5: `lib/eal/common/eal_common_interrupts.c` - FD leak warning may cause false positives**

The added warnings:

```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 will log for any interrupt handle freed with a valid FD. Some drivers intentionally leave the FD set because they do not own it (see the ownership comments added in patch 5). The log should clarify that this is a *potential* leak:

```c
if (rte_intr_fd_get(intr_handle) >= 0)
	EAL_LOG(NOTICE, "Interrupt handle freed with FD %d still set. "
		"This may indicate a resource leak if the FD should have been closed.",
		rte_intr_fd_get(intr_handle));
```

This reduces confusion for users who see the log but know their driver's ownership semantics are correct.

---

## Coding Style and API Compliance

- **Naming, formatting, structure:** All patches follow DPDK conventions. No naming or style violations.
- **Experimental/internal tags:** Correctly used (`rte_intr_fd_close` is `__rte_experimental`, `rte_intr_dev_fd_close` is `__rte_internal`).
- **Release notes:** Appropriately updated.
- **Documentation:** API documentation for the new helpers is clear and correct.

No forbidden tokens, deprecated API usage, or other guideline violations detected.

---

## Conclusion

The series is a valuable cleanup that reduces FD leak risk and clarifies ownership semantics. The main issue is the **error path in patch 2** that could lead to a double-close or use-after-free if `rte_intr_fd_set()` succeeds but a subsequent call fails. This should be fixed before merging. The warnings in patch 3 and patch 5 are minor and can be addressed with documentation or log message refinement.