On Mon, 5 Jan 2026 13:54:41 +0100
David Marchand <[email protected]> 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: [email protected]
>
> Signed-off-by: David Marchand <[email protected]>
> ---
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: [email protected]` | ✓ 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.
