|WARNING| [v6] net/af_xdp: fix shared UMEM refcount corruption

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-20

# DPDK Patch Review

## Summary

This patch fixes critical resource management bugs in the AF_XDP shared UMEM implementation. The fixes address use-after-free, resource leaks, and race conditions. I have identified several issues that need to be addressed.

---

## Errors

### 1. Use-after-free on fq_bufs array after `rte_pktmbuf_free_bulk()`

**Location:** `xsk_configure()` cleanup path

**Issue:**
The `fq_bufs` array is a local stack variable that goes out of scope when the function returns. After calling `rte_pktmbuf_free_bulk(fq_bufs, reserve_size)`, the mbufs are freed, but the array itself is just stack memory and doesn't need explicit cleanup. However, the real issue is that `free_fq_bufs` can be set to `true`, then the first `reserve_fill_queue()` call can fail before clearing it, and we jump to `out_umem` where we free the mbufs--but those mbufs may have already been consumed by `reserve_fill_queue()` on a partial success before it returned an error.

**Current code:**
```c
free_fq_bufs = true;
#endif

/* reserve fill queue of queues not (yet) sharing UMEM */
if (reserve_before) {
	ret = reserve_fill_queue(rxq->umem, reserve_size, fq_bufs, &rxq->fq);
	free_fq_bufs = false;
	if (ret) {
		AF_XDP_LOG_LINE(ERR, "Failed to reserve fill queue.");
		goto out_umem;
	}
```

**Problem:**
If `reserve_fill_queue()` partially consumes some mbufs from `fq_bufs` before failing (e.g., it enqueues some but not all), then `free_fq_bufs` is still `true` and we attempt to free mbufs that were already handed to the fill queue. This is a double-free.

**Fix:**
Set `free_fq_bufs = false` **before** calling `reserve_fill_queue()`, not after on success. Or verify that `reserve_fill_queue()` is truly atomic (all-or-nothing) with respect to the `fq_bufs` array.

---

### 2. Missing error check after `rte_pktmbuf_alloc_bulk()`

**Location:** `xsk_configure()`, line ~1722

**Issue:**
```c
#ifndef ETH_AF_XDP_SHARED_UMEM
ret = rte_pktmbuf_alloc_bulk(rxq->umem->mb_pool, fq_bufs, reserve_size);
if (ret) {
	AF_XDP_LOG_LINE(DEBUG, "Failed to get enough buffers for fq.");
	goto out_umem;
}
free_fq_bufs = true;
#endif
```

The code checks `ret` and jumps to `out_umem` on failure, which is correct. However, the `free_fq_bufs` flag is set to `true` only on the success path (after the `if (ret)` check). This is actually correct--no error here on closer inspection.

**Correction:** No issue here; the `free_fq_bufs = true` is in the success path. Disregard this item.

---

### 3. Race condition on `umem->refcnt` read-check-increment sequence

**Location:** `xdp_umem_configure()`, lines ~1195-1217

**Issue:**
```c
if (umem != NULL) {
	/* Reject sharing once the UMEM is at capacity. */
	if (rte_atomic_load_explicit(&umem->refcnt,
			rte_memory_order_acquire) >= umem->max_xsks) {
		// ... log and return NULL
	}

	AF_XDP_LOG_LINE(INFO, "%s,qid%i sharing UMEM", ...);
	rte_atomic_fetch_add_explicit(&umem->refcnt, 1, rte_memory_order_release);
}
```

Between the `rte_atomic_load_explicit()` check and the `rte_atomic_fetch_add_explicit()` increment, another thread could increment the refcount, allowing the count to exceed `max_xsks`. This is a classic TOCTOU (time-of-check-time-of-use) race.

**Fix:**
Use `rte_atomic_compare_exchange` in a loop, or use `rte_atomic_fetch_add` and check the **old value** returned by the fetch_add to see if it was already at capacity:

```c
uint8_t old_refcnt = rte_atomic_fetch_add_explicit(&umem->refcnt, 1,
	rte_memory_order_release);
if (old_refcnt >= umem->max_xsks) {
	/* We incremented past the limit; undo and fail. */
	rte_atomic_fetch_sub_explicit(&umem->refcnt, 1,
		rte_memory_order_release);
	// ... log and return NULL
}
```

This ensures atomicity: if multiple threads race to increment, only one will observe `old_refcnt < max_xsks` and succeed; the others will see `old_refcnt >= max_xsks` after their increment and will roll back.

---

### 4. Unsigned overflow on refcount decrement check

**Location:** `eth_dev_close()` and `xsk_configure()` cleanup, lines ~1070, ~1856

**Issue:**
```c
if (rte_atomic_fetch_sub_explicit(&rxq->umem->refcnt, 1,
		rte_memory_order_acq_rel) - 1 == 0)
	xdp_umem_destroy(rxq->umem);
```

`rte_atomic_fetch_sub_explicit()` returns the **old value** before the subtraction. Subtracting 1 from that gives the new value. If the refcount is `uint8_t` and somehow underflows (should not happen if the code is correct, but defensive coding), `old_value - 1` wraps to 255 instead of going negative, and `== 0` is false, so we don't destroy. This is actually safe against underflow.

However, a clearer pattern is `old_value == 1` (meaning after the decrement it is zero):

```c
if (rte_atomic_fetch_sub_explicit(&rxq->umem->refcnt, 1,
		rte_memory_order_acq_rel) == 1)
	xdp_umem_destroy(rxq->umem);
```

This avoids the extra subtraction and makes it clearer that we are checking "was this the last reference?"

**Fix suggestion (clarity, not correctness):** Use `== 1` instead of `- 1 == 0`.

---

### 5. NULL dereference if `umem` is still NULL in cleanup path

**Location:** `xsk_configure()` cleanup, line ~1855

**Issue:**
```c
out_umem:
	/* Free fq_bufs that were allocated but never handed to the fill queue. */
	if (free_fq_bufs)
		rte_pktmbuf_free_bulk(fq_bufs, reserve_size);
	if (rte_atomic_fetch_sub_explicit(&rxq->umem->refcnt, 1,
			rte_memory_order_acq_rel) - 1 == 0)
		xdp_umem_destroy(rxq->umem);
```

If `xdp_umem_configure()` returns `NULL` (early in the function, e.g., `get_shared_umem()` fails or allocation fails before the refcount increment), then `rxq->umem` is `NULL` when we jump to `out_umem`. Dereferencing `rxq->umem->refcnt` is a NULL pointer dereference.

**Current mitigation:**
The early return on line ~1710 sets `rxq->umem = NULL` and returns immediately, so we never reach `out_umem` in that case. However, if `xdp_umem_configure()` fails internally (e.g., `xsk_umem__create()` fails after allocating the `umem` struct but before incrementing `refcnt`), the flow is unclear.

**Fix:**
Add a NULL check before dereferencing `rxq->umem` in the cleanup path:

```c
out_umem:
	if (free_fq_bufs)
		rte_pktmbuf_free_bulk(fq_bufs, reserve_size);
	if (rxq->umem != NULL &&
	    rte_atomic_fetch_sub_explicit(&rxq->umem->refcnt, 1,
			rte_memory_order_acq_rel) == 1)
		xdp_umem_destroy(rxq->umem);
	rxq->umem = NULL;
	txq->umem = NULL;
```

Or ensure `xdp_umem_configure()` always increments the refcount before returning a non-NULL `umem`, so the decrement is always safe. Trace the code to confirm.

---

## Warnings

### 6. Inconsistent memory ordering on shared refcount increment

**Location:** `xdp_umem_configure()`, line ~1217

**Issue:**
The patch uses `rte_memory_order_release` on the shared refcount increment:

```c
rte_atomic_fetch_add_explicit(&umem->refcnt, 1, rte_memory_order_release);
```

**Rationale from patch:** "release on the shared increment."

**Concern:**
Release ordering on an increment does NOT synchronize with the acquire load in the capacity check a few lines earlier (they are in the same thread, so no inter-thread ordering is established). The release ordering on the increment ensures that writes **before** the increment are visible to the thread that does the final acquire-release decrement. However, if the UMEM is already initialized and refcnt > 0, there's no data publication happening here--just a reference count bump.

**Correct ordering for a simple refcount increment (no associated data):** `rte_memory_order_relaxed`.

**Correct ordering for a refcount increment that publishes data:** `rte_memory_order_release` (but only if there's data to publish).

**Recommended fix:**
Unless the increment is publishing newly written UMEM state visible to other threads, use `rte_memory_order_relaxed`:

```c
rte_atomic_fetch_add_explicit(&umem->refcnt, 1, rte_memory_order_relaxed);
```

The final decrement already uses `acq_rel` to synchronize the destruction.

---

### 7. Missing bounds check on `reserve_size` variable

**Location:** `xsk_configure()`, line ~1707

**Issue:**
```c
int reserve_size = ETH_AF_XDP_DFLT_NUM_DESCS;
struct rte_mbuf *fq_bufs[reserve_size];
```

`reserve_size` is initialized to a constant (`ETH_AF_XDP_DFLT_NUM_DESCS`), so this is a variable-length array (VLA) on the stack. VLAs are acceptable in DPDK but can cause stack overflow if the constant is very large.

**Recommendation:**
Verify that `ETH_AF_XDP_DFLT_NUM_DESCS` is a reasonable size (e.g., 2048 or 4096). If it's 4096 and `sizeof(struct rte_mbuf *)` is 8, the array is 32 KB, which is large for the stack.

If this is a concern, consider using heap allocation or a fixed-size array with a compile-time assertion that the constant fits.

**Fix suggestion:**
Add a compile-time check or a comment justifying the stack usage:

```c
RTE_BUILD_BUG_ON(ETH_AF_XDP_DFLT_NUM_DESCS > 2048);
```

or allocate on heap if the size is too large.

---

### 8. Potential mempool exhaustion not logged clearly

**Location:** `xdp_umem_configure()`, line ~1199

**Issue:**
When the mempool is too small (max_xsks == 0), the log says:

```c
AF_XDP_LOG_LINE(ERR,
	"%s,qid%i: mempool %s has %u mbufs, "
	"need at least %u to share UMEM",
	internals->if_name, rxq->xsk_queue_idx,
	umem->mb_pool->name,
	umem->mb_pool->populated_size,
	ETH_AF_XDP_NUM_BUFFERS);
```

This is logged only when `max_xsks == 0` (mempool too small to share even one socket). If the mempool is large enough for one socket but not two, and the user tries to add a second queue, the `else` branch logs:

```c
AF_XDP_LOG_LINE(ERR,
	"%s,qid%i: UMEM %s already at max %u sockets",
	...);
```

This is clear enough. However, it might help to also log the mempool size in the "already at max" case, for debugging.

**Suggestion:**
Consider adding the mempool's populated size to the "already at max" log as well.

---

### 9. Documentation wording could be clearer

**Location:** `doc/guides/nics/af_xdp.rst`, line ~102

**Current:**
```rst
The shared mempool must be large enough for every AF_XDP socket sharing the UMEM.
Each socket needs 4096 mbufs for its fill queue, so ``N`` sockets need at least ``4096 * N`` mbufs.
Rx queue setup fails if the mempool is too small to add another socket to the UMEM.
```

**Suggestion:**
Clarify "4096 mbufs" as `ETH_AF_XDP_NUM_BUFFERS` in the code, so if the constant changes, the doc stays accurate. Also, one sentence per line (RST style):

```rst
The shared mempool must be large enough for every AF_XDP socket sharing the UMEM.
Each socket needs 4096 mbufs for its fill queue
(the value of ``ETH_AF_XDP_NUM_BUFFERS`` in the driver), so ``N`` sockets need at least ``4096 * N`` mbufs.
Rx queue setup fails with ``-ENOMEM`` if the mempool is too small to add another socket to the UMEM.
```

---

## Info

### 10. Consider using `RTE_MIN()` inline instead of ternary

**Location:** `xdp_umem_configure()`, line ~1263

**Current:**
```c
umem->max_xsks = RTE_MIN(mb_pool->populated_size / ETH_AF_XDP_NUM_BUFFERS,
			(uint32_t)UINT8_MAX);
```

This is already using `RTE_MIN()`, which is correct. No issue here.

---

### 11. Verbose logging on every shared queue setup

**Location:** `xdp_umem_configure()`, line ~1215

**Current:**
```c
AF_XDP_LOG_LINE(INFO, "%s,qid%i sharing UMEM", ...);
```

This logs at INFO level every time a queue shares the UMEM. If many queues are set up, this could be verbose. Consider DEBUG level or rate-limiting.

**Suggestion:** Change to DEBUG level unless it's important for users to see.

---

## Summary of Required Fixes

**Must fix (Errors):**

1. **TOCTOU race on refcount increment** (Error #3): Use `fetch_add` and check the old value atomically.
2. **NULL dereference in cleanup path** (Error #5): Add NULL check before `rxq->umem->refcnt`.
3. **Double-free risk if `reserve_fill_queue()` partially consumes `fq_bufs`** (Error #1): Verify `reserve_fill_queue()` atomicity or set `free_fq_bufs = false` before calling it.

**Should fix (Warnings):**

4. **Memory ordering on refcount increment** (Warning #6): Use `relaxed` unless publishing data.
5. **Refcount comparison clarity** (Error #4, reclassified as clarity): Use `== 1` instead of `- 1 ==