Re: [PATCH] net/af_xdp: fix external mbuf transmit

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  5 Jan 2026 13:54:41 +0100
David Marchand <david.marchand@redhat.com> wrote:

> A mbuf may be pointing at external buffer like when forwarding a TSO
> packet coming from a vhost-user port in OVS (see
> RTE_VHOST_USER_EXTBUF_SUPPORT).
> 
> Checking the mbuf mempool to trigger the zero copy optimisation is
> broken: the driver will (at best) read 0s or (at worse) crash when
> reading more data than what the direct buffer can contain.
> 
> Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Looks good to me and Claude ;-) but he has more to say.
Queued to next-net

## DPDK Patch Review: net/af_xdp: fix external mbuf transmit

### Summary

This patch fixes a bug in the AF_XDP driver's zero-copy transmit path where mbufs pointing to external buffers (e.g., from vhost-user with `RTE_VHOST_USER_EXTBUF_SUPPORT`) could cause data corruption or crashes. The fix adds an `RTE_MBUF_DIRECT()` check before using the zero-copy optimization.

---

### Commit Message Review

| Criterion | Status | Details |
|-----------|--------|---------|
| Subject length | ✓ Pass | 38 characters (limit: 60) |
| Subject format | ✓ Pass | `net/af_xdp:` prefix is correct for driver |
| Subject case | ✓ Pass | Lowercase after colon |
| Imperative mood | ✓ Pass | "fix" not "fixes" or "fixed" |
| No trailing period | ✓ Pass | |
| Body line wrap | ✓ Pass | All lines under 75 characters |
| Body doesn't start with "It" | ✓ Pass | Starts with "A mbuf" |
| `Fixes:` tag | ✓ Pass | 12-char SHA: `d8a210774e1d` |
| `Cc: stable@dpdk.org` | ✓ Pass | Present for bug fix |
| `Signed-off-by:` | ✓ Pass | Present with real name and email |
| Tag order | ✓ Pass | Fixes → Cc → blank line → Signed-off-by |

---

### Code Review

**Change location:** `drivers/net/af_xdp/rte_eth_af_xdp.c`, line 608 (in `af_xdp_tx_zc()`)

```c
// Before:
if (mbuf->pool == umem->mb_pool) {

// After:
if (RTE_MBUF_DIRECT(mbuf) && mbuf->pool == umem->mb_pool) {
```

| Criterion | Status | Details |
|-----------|--------|---------|
| Line length | ✓ Pass | Well under 100 characters |
| Macro naming | ✓ Pass | `RTE_MBUF_DIRECT` uses correct `RTE_` prefix |
| Technical correctness | ✓ Pass | Properly gates zero-copy path |
| Minimal change | ✓ Pass | Single-line fix, targeted |

#### Warning (Minor)

**Implicit boolean comparison:** The `RTE_MBUF_DIRECT(mbuf)` macro returns an integer-like value (0 or non-zero). Per DPDK guidelines, explicit comparison with zero is preferred for integers:

```c
// Suggested (strict interpretation):
if (RTE_MBUF_DIRECT(mbuf) != 0 && mbuf->pool == umem->mb_pool) {
```

However, this macro is semantically boolean and is commonly used in direct form throughout DPDK. This is a stylistic preference, not a blocking issue.

---

### Technical Analysis

The fix is correct. Looking at the `af_xdp_tx_zc()` function in the provided source:

1. **The bug:** Line 624 in the original code only checks `mbuf->pool == umem->mb_pool` to decide if zero-copy can be used. This fails for indirect/external mbufs where `mbuf->buf_addr` points to external memory, not the pool's memory region.

2. **The consequence:** When zero-copy is incorrectly triggered, `reserve_and_fill()` computes addresses relative to `umem->buffer`, which produces invalid addresses for external buffer data.

3. **The fix:** Adding `RTE_MBUF_DIRECT(mbuf)` ensures the mbuf data resides in its own attached buffer, making the pool check meaningful.

4. **Fallback path:** When the check fails, the code correctly falls through to the else branch (lines 633-647) which allocates a local mbuf and performs a copy.

---

### Verdict

| Severity | Count | Items |
|----------|-------|-------|
| **Error** | 0 | — |
| **Warning** | 1 | Implicit boolean check on `RTE_MBUF_DIRECT()` |
| **Info** | 0 | — |

**Recommendation:** **Accept** — This is a well-crafted bug fix. The commit message is clear, properly attributed, and follows DPDK conventions. The code change is minimal, correct, and addresses a real crash/corruption scenario. The warning about implicit boolean comparison is minor and matches existing DPDK patterns.