DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] net/af_xdp: fix external mbuf transmit
@ 2026-01-05 12:54 David Marchand
  2026-01-12  1:03 ` Stephen Hemminger
  0 siblings, 1 reply; 3+ messages in thread
From: David Marchand @ 2026-01-05 12:54 UTC (permalink / raw)
  To: dev; +Cc: stable, Ciara Loftus, Maryam Tahhan, Xiaolong Ye, Kevin Laatz

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>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index f2a8fb95d1..10dbcf1333 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -608,7 +608,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	for (i = 0; i < nb_pkts; i++) {
 		mbuf = bufs[i];
 
-		if (mbuf->pool == umem->mb_pool) {
+		if (RTE_MBUF_DIRECT(mbuf) && mbuf->pool == umem->mb_pool) {
 			desc = reserve_and_fill(txq, mbuf, umem, NULL);
 			if (!desc) {
 				kick_tx(txq, cq);
-- 
2.52.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net/af_xdp: fix external mbuf transmit
  2026-01-05 12:54 [PATCH] net/af_xdp: fix external mbuf transmit David Marchand
@ 2026-01-12  1:03 ` Stephen Hemminger
  2026-01-12  7:58   ` David Marchand
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Hemminger @ 2026-01-12  1:03 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, stable, Ciara Loftus, Maryam Tahhan, Xiaolong Ye, Kevin Laatz

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net/af_xdp: fix external mbuf transmit
  2026-01-12  1:03 ` Stephen Hemminger
@ 2026-01-12  7:58   ` David Marchand
  0 siblings, 0 replies; 3+ messages in thread
From: David Marchand @ 2026-01-12  7:58 UTC (permalink / raw)
  To: Stephen Hemminger, Morten Brørup
  Cc: dev, stable, Ciara Loftus, Maryam Tahhan, Xiaolong Ye,
	Kevin Laatz, Thomas Monjalon

Hello,

On Mon, 12 Jan 2026 at 02:04, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> #### 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.

All callers in DPDK are using this macro as a boolean, and I found no
opensource project doing differently.
I would either fix the macro or change nothing at all.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-01-12  7:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-01-05 12:54 [PATCH] net/af_xdp: fix external mbuf transmit David Marchand
2026-01-12  1:03 ` Stephen Hemminger
2026-01-12  7:58   ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).