|WARNING| [v7, 6/6] eal: fix async IPC callback not fired when no peers

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/6: eal: fix wrong log message in async IPC request

**No issues found.**

---

## Patch 2/6: eal: use request ID instead of pointers

### Errors

1. **Race condition on shared variable without atomics**

`next_request_id` is a global variable incremented without synchronization:

```c
static unsigned long next_request_id;
...
id = ++next_request_id;
```

This is a data race. Multiple threads calling `mp_request_async()` or `rte_mp_request_async()` concurrently (both hold `pending_requests.lock` but that doesn't prevent concurrent increments from separate calls) will cause undefined behavior.

**Fix:** Use atomic increment:
```c
id = rte_atomic_fetch_add_explicit(&next_request_id, 1, rte_memory_order_relaxed) + 1;
```

Or declare `next_request_id` as `rte_atomic64_t` and use `rte_atomic_fetch_add_explicit()`.

---

## Patch 3/6: eal: avoid deadlock in async IPC alarm callback

**No issues found.**

The removal of `rte_eal_alarm_cancel()` from the callback path correctly avoids the deadlock. The comment explains that the ID-based lookup makes stale callbacks safe.

---

## Patch 4/6: eal: fix async IPC memory leaks on partial failure

### Errors

1. **Potential use-after-free in rollback loop**

The rollback loop frees `r->request` (which equals `copy`) for each matching request:

```c
for (r = TAILQ_FIRST(&pending_requests.requests);
        r != NULL; r = next) {
    next = TAILQ_NEXT(r, next);
    if (r->type == REQUEST_TYPE_ASYNC &&
            r->async.param == param) {
        TAILQ_REMOVE(&pending_requests.requests, r, next);
        free(r->reply);
        /* r->request == copy, freed below after the loop */
        free(r);
    }
}
```

The comment says `r->request == copy` will be freed after the loop, but `r->request` is never freed inside the loop. If multiple requests were queued (multiple calls to `mp_request_async()` in the loop over `mp_dirs`), they **all** point to the same `copy` buffer. The first iteration through the rollback loop will leave `r->request` dangling in `r`, but since `r` itself is freed, this is not a use-after-free **yet**.

However, the **real issue** is that each `pending_request` has `pending_req->request = req;` where `req` is the `copy` pointer. When you `free(r)`, you lose the pointer, but you never freed `r->request`. Since all requests share the same `copy`, you should only free `copy` once at the end (which the code does). But the comment is misleading: `r->request` is not "freed below after the loop" -- it's the shared `copy` pointer that's freed once. The per-request `r` structures are freed in the loop, but their `->request` pointers are all aliases to the same `copy`.

**This is actually correct** on closer inspection: each `r->request` points to `copy`, and `copy` is freed once at the end (`if (ret != 0) { free(param); free(copy); }`). The comment should clarify that `r->request` is an alias, not a separate allocation.

**Revised assessment:** The code is correct. The comment is slightly confusing but not wrong. **No error.**

### Warnings

1. **Misleading comment**

The comment `/* r->request == copy, freed below after the loop */` could be clearer. Suggest:
```c
/* r->request points to shared 'copy', freed once below */
```

---

## Patch 5/6: eal: fix memory leak in async IPC secondary path

**No issues found.**

The fix correctly frees `dummy` on the success path when it was allocated but not used.

---

## Patch 6/6: eal: fix async IPC callback not fired when no peers

### Errors

1. **Missing error path cleanup (memory leak)**

In the secondary process path, when `rte_eal_alarm_set()` fails, the code removes `dummy` from the queue and sets `dummy_used = false`, but does not release the lock before jumping to `unlock_fail`:

```c
if (rte_eal_alarm_set(1, async_reply_handle,
        (void *)(uintptr_t)dummy->id) < 0) {
    EAL_LOG(ERR, "Fail to set alarm for dummy request");
    TAILQ_REMOVE(&pending_requests.requests, dummy, next);
    dummy_used = false;
    ret = -1;
    goto unlock_fail;
}
```

Then at `unlock_fail`:
```c
pthread_mutex_unlock(&pending_requests.lock);
/* if we couldn't send anything, clean up */
if (ret != 0)
    goto fail;
```

This jumps to `fail:`, which only frees `dummy` if `dummy_used` is false (which it is), but it does NOT free `copy` or `param`. In the secondary path, `copy` and `param` are allocated before this point, and on failure they are never freed.

**Fix:** At label `fail:` (which is common to both primary and secondary paths), ensure `copy` and `param` are freed:

```c
fail:
    free(dummy);
    free(param);
    free(copy);
    return -1;
```

But wait -- looking at the full context, the `fail:` label at the end of the function is:

```c
fail:
    free(dummy);
    free(param);
    free(copy);
    return -1;
```

So the fix is already applied at the end of patch 4 (the `if (ret != 0) { free(param); free(copy); }` block). But in patch 6, the secondary path's new error path (`goto unlock_fail;`) eventually reaches `goto fail;`, which will free `copy` and `param`. So **this is actually correct** as long as `copy` and `param` are freed at `fail:`.

Let me re-check: Patch 4 added:
```c
/* if nothing was sent, nobody owns copy/param */
if (ret != 0) {
    free(param);
    free(copy);
}
```

And that block is **before** `return ret;` at the end of the function. So when patch 6 jumps to `fail:`, it will free `dummy`, `param`, and `copy` (from the patch 4 additions).

**Actually, on closer reading:** The `fail:` label in patch 6 is inside the `if (rte_eal_process_type() == RTE_PROC_SECONDARY)` block. Let me trace the full path:

```c
if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
    ret = mp_request_async(eal_mp_socket_path(), copy, param, ts);

    if (ret == 0 && reply->nb_sent == 0) {
        TAILQ_INSERT_TAIL(&pending_requests.requests, dummy, next);
        dummy_used = true;
        if (rte_eal_alarm_set(1, async_reply_handle,
                (void *)(uintptr_t)dummy->id) < 0) {
            EAL_LOG(ERR, "Fail to set alarm for dummy request");
            TAILQ_REMOVE(&pending_requests.requests, dummy, next);
            dummy_used = false;
            ret = -1;
            goto unlock_fail;
        }
    }

    pthread_mutex_unlock(&pending_requests.lock);
    /* if we couldn't send anything, clean up */
    if (ret != 0)
        goto fail;
    if (!dummy_used)
        free(dummy);
    return 0;
}
```

Then later:
```c
fail:
    free(dummy);
    free(param);
    free(copy);
    return -1;
```

So when `rte_eal_alarm_set()` fails, `ret = -1`, we jump to `unlock_fail:` which unlocks, then checks `if (ret != 0)` and jumps to `fail:`, which frees everything. **This is correct.**

**Revised assessment:** No error.

2. **Similar error path in primary process section**

The same pattern occurs in the primary process path (second instance of the fix):

```c
if (ret == 0 && reply->nb_sent == 0) {
    TAILQ_INSERT_HEAD(&pending_requests.requests, dummy, next);
    dummy_used = true;

    if (rte_eal_alarm_set(1, async_reply_handle,
            (void *)(uintptr_t)dummy->id) < 0) {
        EAL_LOG(ERR, "Fail to set alarm for dummy request");
        TAILQ_REMOVE(&pending_requests.requests, dummy, next);
        dummy_used = false;
        ret = -1;
        goto closedir_fail;
    }
}
```

Then:
```c
closedir_fail:
    closedir(mp_dir);
unlock_fail:
    pthread_mutex_unlock(&pending_requests.lock);
fail:
    /* if dummy was unused, free it */
    if (!dummy_used)
        free(dummy);
    /* if nothing was sent, nobody owns copy/param */
    if (ret != 0) {
        free(param);
        free(copy);
    }

    return ret;
```

So when `rte_eal_alarm_set()` fails in the primary path, `ret = -1`, we jump to `closedir_fail:` -> `unlock_fail:` -> `fail:` which checks `if (ret != 0)` and frees `param` and `copy`. **This is also correct.**

**Final assessment for patch 6:** No correctness errors.

---

## Summary

### Errors

**Patch 2:**
- `next_request_id` incremented without atomic operations, causing data race when multiple threads call async IPC functions concurrently.

### Warnings

**Patch 4:**
- Misleading comment in rollback loop; clarify that `r->request` is an alias to shared `copy`.

---

## Final Check

**Pass 1 (Correctness bugs):**
- Identified data race on `next_request_id` (patch 2).
- Traced all error paths in patches 4 and 6; no resource leaks.

**Pass 2 (Style/process false positives):**
- Removed initially flagged "use-after-free" in patch 4 after determining the code is correct.
- Removed initially flagged "memory leak" in patch 6 after tracing the full error path.
- Kept the comment clarity suggestion in patch 4 as a minor warning.