|WARNING| [v2,2/2] net/tap: fix use after free on interrupt handle

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-06

# DPDK Patch Review

## PATCH 1/2: net/tap: fix FD leak on netns change

### Errors

**Netlink socket close/assignment before alarm registration**

The patch closes the old netlink socket (`tap_nl_final(pmd->nlsk_fd)`) and assigns the new one (`pmd->nlsk_fd = new_nlsk_fd`) before scheduling the alarm that will recreate the LSC interrupt handle. If the alarm fails to register (`rte_eal_alarm_set()` returns < 0), the driver continues with the new netlink socket installed but the LSC interrupt handle still pointing to the old (now closed) netlink socket fd. Any subsequent LSC event will attempt to use the freed fd, causing use-after-free or fd reuse bugs.

```c
/* Current code at line 1887-1893 */
if (pmd->nlsk_fd >= 0)
    tap_nl_final(pmd->nlsk_fd);
pmd->nlsk_fd = new_nlsk_fd;

/* Defer LSC re-registration outside callback context */
if (rte_eal_alarm_set(1, tap_lsc_register_alarm, dev) < 0)
    TAP_LOG(ERR, "%s: failed to schedule LSC re-registration",
        pmd->name);
```

**Fix:** Only close the old netlink socket and assign the new one after confirming the alarm registration succeeded:

```c
/* Defer LSC re-registration outside callback context */
if (rte_eal_alarm_set(1, tap_lsc_register_alarm, dev) < 0) {
    TAP_LOG(ERR, "%s: failed to schedule LSC re-registration",
        pmd->name);
    if (new_nlsk_fd >= 0)
        tap_nl_final(new_nlsk_fd);
    return -1;
}

if (pmd->nlsk_fd >= 0)
    tap_nl_final(pmd->nlsk_fd);
pmd->nlsk_fd = new_nlsk_fd;
```

---

**Missing error handling for `tap_nl_init()` failure**

At line 1866, `tap_nl_init(0)` can fail and return `-1`. The code does not check this before passing `new_nlsk_fd` to `tap_nl_final()` at line 1886 and assigning it to `pmd->nlsk_fd` at line 1887. Calling `tap_nl_final(-1)` or storing `-1` as the netlink socket fd will cause subsequent netlink operations to fail or use an invalid fd.

```c
/* Recreate netlink socket in new namespace */
new_nlsk_fd = tap_nl_init(0);
```

**Fix:** Check the return value and handle failure:

```c
new_nlsk_fd = tap_nl_init(0);
if (new_nlsk_fd < 0) {
    TAP_LOG(ERR, "%s: failed to recreate netlink socket in new namespace",
        pmd->name);
    tap_dev_ns_restore(pmd, orig_netns_fd);
    return -1;
}
```

---

**Resource leak on `tap_dev_ns_switch()` early return in `tap_lsc_register_alarm()`**

At line 2007 in `tap_lsc_register_alarm()`, if `tap_dev_ns_switch()` fails and returns `-1`, the function returns early without calling `tap_dev_ns_restore()`. However, `tap_dev_ns_switch()` may have successfully opened `orig_netns_fd` before the `setns()` call fails (the failure happens at line 1824-1829 in `tap_dev_ns_switch()`). When `setns()` fails, the function closes `netns_fd` and `orig_netns_fd` at lines 1827-1828, so the current code is actually correct on that path.

But if `tap_dev_ns_switch()` succeeds in switching namespaces and then some later operation fails (e.g., `tap_lsc_intr_handle_set()` at line 2011), the code still calls `tap_dev_ns_restore()` at line 2013, which is correct.

Actually, re-reading the code: `tap_dev_ns_switch()` returns `-1` if either `ioctl()` fails (line 1810), or `open()` fails (line 1815), or `setns()` fails (line 1824). In all three cases, it cleans up any fds it opened before returning. So there is no leak here. This is not a bug.

---

### Warnings

**Duplicate alarm cancellation**

The patch adds `rte_eal_alarm_cancel(tap_lsc_register_alarm, dev)` at both line 1970 (in the cleanup path of `tap_lsc_intr_handle_set()`) and line 1988 (after the unregister loop). The second call at line 1988 is redundant: if the function reaches line 1988, it has already executed line 1970 (either from the `set == 0` path at line 1969, or from a failed LSC setup at line 1950 which jumps to `clean:` at line 1969). Canceling the same alarm twice is harmless but unnecessary.

```c
clean:
	rte_eal_alarm_cancel(tap_lsc_register_alarm, dev);  /* line 1970 */
	if (rte_intr_fd_get(pmd->intr_handle) < 0)
		return 0;

	do {
		ret = rte_intr_callback_unregister(pmd->intr_handle,
			tap_dev_intr_handler, dev);
		/* ... */
	} while (true);

	rte_eal_alarm_cancel(tap_lsc_register_alarm, dev);  /* line 1988 */
```

**Suggested fix:** Remove the second cancel at line 1988, or restructure so each path cancels exactly once.

---

## PATCH 2/2: net/tap: fix use after free on interrupt handle

### Info

The patch adds `tap_intr_handle_set(dev, 0)` to `tap_dev_close()` to unregister interrupt callbacks before the port closes. This prevents use-after-free of the interrupt handle after the driver change in commit c663018eb7f3. The fix is straightforward and correct.

---

## Summary

**Patch 1/2** has two **Errors** requiring fixes before merge:
1. Netlink socket close/assignment must happen only after alarm registration succeeds
2. Missing error check on `tap_nl_init()` return value

**Patch 1/2** has one **Warning**:
- Duplicate `rte_eal_alarm_cancel()` call (harmless but redundant)

**Patch 2/2** is clean.

Review-Result: ERROR