|SUCCESS| [v1, 1/1] eal/linux: fix eventfd leaks when enabling interrupts

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171887

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

None identified.

## WARNINGS

**1. Missing bounds check on counter variable `i` in cleanup path**

The cleanup loop `while (i > 0)` decrements `i` before use but relies on `i` having been incremented up to `n` in the allocation loop. If the allocation loop exits early due to `eventfd()` failure before any successful allocations, `i` will be 0 and the cleanup loop will not execute, which is correct. However, if `rte_intr_efds_index_set()` fails after `eventfd()` succeeds, `i` is not incremented in that iteration, so the cleanup loop will skip the fd that was just created and not yet stored in the handle.

```c
fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
if (fd < 0) {
	ret = -errno;
	/* ... */
	goto error;
}

if (rte_intr_efds_index_set(intr_handle, i, fd)) {
	ret = -rte_errno;
	close(fd);          /* fd is closed here */
	goto error;         /* but i is not incremented, so cleanup loop won't touch it */
}
```

This is actually correct as-is because the `close(fd)` happens before `goto error` when `rte_intr_efds_index_set()` fails. The fd that failed to be stored is closed immediately, and the cleanup loop only needs to handle successfully stored fds (those where `i` was incremented after a successful set). 

Actually on closer inspection: the loop counter `i` is incremented by the `for` statement after each iteration body completes. If `rte_intr_efds_index_set()` fails, the `goto error` prevents the loop increment, so `i` remains at the current iteration value. The fd is closed before the goto. The cleanup loop then processes `0..i-1`, which are the fds that were successfully set. This is correct.

However, there is a subtle issue: if `rte_intr_efds_index_set()` succeeds but a subsequent `rte_intr_nb_efd_set()` or `rte_intr_max_intr_set()` fails after the allocation loop completes, the cleanup loop will process `0..n-1` (since the for loop ran to completion with `i` reaching `n`), but those calls happen outside the loop, so `i` will have the value `n` at the error label. The cleanup logic is correct in this case too.

Upon final review of the control flow: the code is correct. Removing this item.

**2. EBUSY return is the correct HTTP-style semantic but inconsistent with DPDK error return patterns elsewhere**

DPDK interrupt functions typically return `-errno` values for failures. Returning `-EBUSY` for "already enabled" is semantically correct and matches Linux kernel conventions for resource-already-acquired errors. This is an acceptable choice and consistent with the existing error return convention in this function (`-errno`, `-rte_errno`). Not a violation.

Removing this item.

## INFO

**1. Cleanup loop could use for loop for clarity**

The cleanup path uses `while (i > 0) { i--; ... }` to iterate backward through allocated fds. A `for` loop would make the iteration direction and bounds more explicit:

```c
error:
	for (uint32_t cleanup_idx = 0; cleanup_idx < i; cleanup_idx++) {
		close(rte_intr_efds_index_get(intr_handle, cleanup_idx));
		rte_intr_efds_index_set(intr_handle, cleanup_idx, -1);
	}
	rte_intr_nb_efd_set(intr_handle, 0);

	return ret;
```

However, the existing `while` pattern is also clear and commonly used in DPDK for cleanup paths. This is a style preference, not a defect.

**2. The patch fixes a correctness bug (resource leak) and properly handles partial allocation cleanup**

The patch correctly identifies and fixes two resource leak scenarios:
- Partial allocation failure leaving eventfds open (because `nb_efd` is set only after all allocations succeed, so `rte_intr_efd_disable()` cannot clean up)
- Repeated enable calls overwriting existing eventfd entries without closing them

The fix is sound:
- Rejects repeated enable with `-EBUSY` before modifying state
- On failure, closes all eventfds created in the current call
- Clears the entries and resets `nb_efd` to 0
- Preserves the original error code in `ret`

The cleanup loop correctly handles the case where `rte_intr_efds_index_set()` fails: the fd is closed immediately before the `goto`, so only successfully-set fds need to be cleaned up by the error path. The loop variable `i` will be positioned correctly to iterate over `0..i-1` (the successfully set entries).

Review-Result: CLEAN