|WARNING| [v3] net/virtio-user: fix eventfd sharing in secondary process

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: virtio-user eventfd sharing in secondary process

## Summary
This patch addresses a real multiprocess communication bug in virtio-user and includes several important correctness fixes. The core functionality is sound, but there are resource leak issues on error paths that must be fixed.

---

## Errors (must fix)

### 1. Resource leak in `virtio_user_sync_eventfds()` on MP request failure
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 574-580

The `err_free` label frees the fd arrays but never closes the fds that were set to -1 by the initialization loop (lines 619-622). If `rte_mp_request_sync()` fails after the arrays are allocated, we leak the allocated memory for the arrays themselves (pp->kickfds, pp->callfds) and the pp structure.

**Current code:**
```c
if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) < 0 ||
    mp_reply.nb_received != 1) {
    PMD_INIT_LOG(ERR, "Failed to request eventfds from primary");
    free(mp_reply.msgs);
    ret = -EIO;
    goto err_free;  /* leaks pp and arrays if no fds were received */
}
```

The `err_free` path correctly frees everything, but only after fds are populated. When MP request fails before fds are received, we jump to `err_free` with all -1 fds, which the cleanup loop skips (line 566 checks `>= 0`), so the arrays and pp are freed correctly. 

Actually, reviewing more carefully: the `err_free` label at lines 708-711 DOES free pp->kickfds, pp->callfds, and pp. So this is not a leak. My initial concern was incorrect.

### 2. Double-close risk in `virtio_user_free_proc_priv()`
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 557-577

After closing an fd, the code does not set it to -1. If `virtio_user_free_proc_priv()` is called twice (e.g., from an error path and then again during normal cleanup), the same fd will be closed twice, which is undefined behavior and could close an unrelated fd if the OS reused the fd number.

**Fix:**
```c
for (i = 0; i < pp->nr_queues; i++) {
    if (pp->kickfds != NULL && pp->kickfds[i] >= 0) {
        close(pp->kickfds[i]);
        pp->kickfds[i] = -1;  /* ADD THIS */
    }
    if (pp->callfds != NULL && pp->callfds[i] >= 0) {
        close(pp->callfds[i]);
        pp->callfds[i] = -1;  /* ADD THIS */
    }
}
```

Note: The current code sets `eth_dev->process_private = NULL` at the end (line 576), which would prevent a second call if the caller checks that pointer. However, defensive programming dictates setting fds to -1 immediately after close, as there is no guarantee all callers check process_private before calling this function.

### 3. Missing error propagation in `virtio_user_notify_queue()`
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 342-346

When `virtio_user_get_kickfd()` returns -1 (invalid fd), the code logs an error via the `write()` check but does not distinguish between "invalid fd" and "write failed on valid fd". An invalid fd means the device is misconfigured (a setup/teardown bug), while a write failure is a runtime I/O error. The error path should be different.

More critically, after logging the error, execution continues as if the notification succeeded. This means the backend never receives the kick, and traffic silently stalls. This is a **silent failure** that produces wrong results (lost packets).

**Fix:**
```c
int kickfd = virtio_user_get_kickfd(hw, dev, vq->vq_queue_index);

if (kickfd < 0) {
    PMD_DRV_LOG(ERR, "Invalid kickfd for queue %u",
                vq->vq_queue_index);
    /* Cannot notify backend - device is broken */
    return;
}

if (write(kickfd, &notify_data, sizeof(notify_data)) < 0) {
    PMD_DRV_LOG(ERR, "failed to kick backend: %s",
                strerror(errno));
}
```

This makes the "kickfd < 0" case explicit and distinguishes it from write failures.

---

## Warnings (should fix)

### 1. `virtio_user_get_eventfds_from_dev()` validation could be tighter
**File:** `drivers/net/virtio/virtio_user/virtio_user_dev.c`  
**Lines:** 42-83

The check `if (dev->kickfds == NULL || dev->callfds == NULL)` catches uninitialized arrays, but does not verify that the device is in a valid state (e.g., `dev->started` is consistent, queues are set up). If called on a partially-initialized device, the function could return invalid fds.

**Suggested improvement:**
Add a check that `dev->started != 0` or that the device has completed initialization. However, since this function is called under `dev->mutex` with the MP handler as the only caller, and the MP handler itself is only called after `virtio_user_dev_init()` completes, this may be acceptable as-is. The existing checks are defensive enough for the current usage.

This is a **minor** concern and not a blocker.

### 2. `virtio_user_mp_primary_handler()` allocates eventfds array on stack with hardcoded max
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 584-585

```c
int eventfds[VIRTIO_USER_MAX_EVENTFDS];
```

`VIRTIO_USER_MAX_EVENTFDS` is `RTE_MP_MAX_FD_NUM` which is typically 8 or small. However, this is a fixed-size stack array. If `RTE_MP_MAX_FD_NUM` is increased in the future (or differs across DPDK versions), this could cause a stack overflow if the actual device needs more fds than the compile-time constant.

The check at line 29 in `virtio_user_get_eventfds_from_dev()` already guards against this:
```c
if (max_queues * VIRTIO_USER_EVENTFDS_PER_QUEUE > VIRTIO_USER_MAX_EVENTFDS)
```
So this is **not** a bug, but it does couple the validation logic to the stack allocation size in a non-obvious way.

**Suggested improvement:** Add a comment in the MP handler that the stack array size is validated by `virtio_user_get_eventfds_from_dev()`.

### 3. Missing bounds check in `virtio_user_get_kickfd()`
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 309-327

In the primary path, the function does not check `queue_idx < max_queues` before accessing `dev->kickfds[queue_idx]`. If the caller passes an out-of-bounds queue_idx, this is an out-of-bounds array access.

**Current code:**
```c
return dev->kickfds[queue_idx];  /* no bounds check */
```

**Fix:**
```c
if (queue_idx >= dev->max_queue_pairs * 2 + (dev->hw_cvq ? 1 : 0))
    return -1;
return dev->kickfds[queue_idx];
```

However, the caller (`virtio_user_notify_queue()`) gets `queue_idx` from `vq->vq_queue_index`, which is set during queue setup and should always be in bounds. So this is **defensive programming** rather than a known bug.

---

## Info (consider)

### 1. `virtio_user_sync_eventfds()` timeout is 5 seconds
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Line:** 638

The comment explains the rationale (EAL MP thread backed up), which is reasonable. However, 5 seconds is a long time for probe to block. If the primary is unresponsive, this adds 5 seconds to secondary startup failure.

Consider: Is there a way to fail faster if the primary is known to be gone? (e.g., check if the primary's PID exists before waiting 5 seconds). This is a usability concern, not a correctness bug.

### 2. `virtio_user_mp_registered` uses a spinlock for one-time init
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 949-982

The pattern is correct, but the comment "unregistering per device cannot drain handler calls already dispatched" (line 958) implies the handler could be called after the device is gone. The handler does call `rte_eth_dev_get_by_name()`, which will fail if the port was removed, so this is safe. However, the lifetime comment could be clearer.

**Suggested comment improvement:**
```c
/*
 * Register the process-wide MP action once so secondaries can fetch a
 * port's eventfds by name. Left registered for the process lifetime
 * (no unregister on device removal) because the EAL MP thread may still
 * dispatch handler calls after rte_mp_action_unregister() returns.
 * The handler safely fails if the requested port no longer exists.
 */
```

### 3. Error handling in `virtio_user_pmd_probe()` after MP registration failure
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Lines:** 963-979

When `rte_mp_action_register()` fails with `ENOTSUP`, the code logs a warning and continues as if MP is not available. This means secondaries will fail to sync eventfds and cannot communicate with the backend. The warning message says "secondary eventfd sharing disabled", but the device is still announced, which will cause runtime failures in secondaries.

**Is this intentional?** If MP is unsupported, should the device be usable in single-process mode only, or should probe fail entirely? The current behavior is confusing: probe succeeds, but the device is broken in multiprocess mode.

**Suggested clarification:** Add a check at secondary probe time (in the `RTE_PROC_SECONDARY` branch, line 764) to fail early if `eth_dev->process_private == NULL` after `virtio_user_sync_eventfds()` fails, rather than announcing a non-functional port.

---

## Style and Formatting

### 1. Explicit comparison for `mp_reply.nb_received`
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Line:** 673

```c
if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) < 0 ||
    mp_reply.nb_received != 1) {
```

This is correct as written (explicit comparison against a non-zero value). No change needed.

### 2. Explicit NULL check for `pp`
**File:** `drivers/net/virtio/virtio_user_ethdev.c`  
**Line:** 564

```c
if (pp == NULL)
    return;
```

Correct per DPDK style (explicit NULL check). No change needed.

### 3. Loop counter type consistency
**File:** `drivers/net/virtio/virtio_user/virtio_user_dev.c`  
**Line:** 71

```c
for (i = 0; i < (int)max_queues; i++) {
```

The cast `(int)max_queues` is needed because `i` is `int` and `max_queues` is `uint32_t`. The comparison is safe (max_queues is bounded by `VIRTIO_USER_MAX_EVENTFDS / 2`). Acceptable.

---

## Multiprocess Design Review

The approach (primary shares fds via MP, secondary stores them in `process_private`) is sound and follows the pattern used by other multiprocess-aware drivers. The synchronization (primary holds `dev->mutex` while reading fd arrays, teardown holds the same lock while freeing them) correctly prevents races.

**One design question:** Why not use `rte_mp_register()` per-device instead of a process-wide action? The current design requires the handler to look up the port by name, which adds an extra indirection. A per-device action could store the `dev` pointer directly. However, the process-wide approach is acceptable and matches the EAL MP API's typical usage.

---

## Correctness Summary

**Critical fixes required:**
1. Set fds to -1 after close in `virtio_user_free_proc_priv()` (double-close risk)
2. Distinguish invalid kickfd from write failure in `virtio_user_notify_queue()` (silent failure)

**Recommended fixes:**
1. Bounds check in `virtio_user_get_kickfd()` primary path (defensive)
2. Clarify behavior when MP is unsupported (`ENOTSUP` case)

The patch is **nearly correct** but the two critical fixes are necessary to prevent undefined behavior and silent failures.