* Re: [PATCH] net/idpf: fix Tx payload corruption in split queue
2026-09-30 4:46 [PATCH] net/idpf: fix Tx payload corruption in split queue Shaiq Wani
@ 2026-10-06 18:22 ` Bruce Richardson
2026-10-07 10:40 ` [PATCH v2] net/intel: fix idpf " Shaiq Wani
1 sibling, 0 replies; 3+ messages in thread
From: Bruce Richardson @ 2026-10-06 18:22 UTC (permalink / raw)
To: Shaiq Wani; +Cc: dev, aman.deep.singh, stable
On Wed, Sep 30, 2026 at 10:16:30AM +0530, Shaiq Wani wrote:
> Observed on ACC while running NVMe traffic tests: under sustained
> high-throughput split-queue Tx, payload bytes on the wire did not
> match what the application submitted, while headers and checksums
> looked valid.
>
> The RS-completion path was freeing mbufs by walking sw_ring[] slots
> between first_id and the EOP sw_id. sw_ring[] slots get reclaimed
> by RE and reused by subsequent submits well before RS lands, so the
> walker was freeing mbufs still in flight for later packets, whose
> buffers then got recycled and overwritten mid-DMA.
>
> Fix by tracking completion ownership in a shadow ring indexed by a
> software-defined compl_tag stamped on the EOP descriptor and echoed
> back by HW on RS. sw_ring[].mbuf and .first_id are no longer touched
> on the Tx completion path.
>
> Fixes: 96cc9b6ea60c ("net/idpf: fix multi-segment mbuf leak in split Tx path")
> Cc: stable@dpdk.org
>
> Signed-off-by: Shaiq Wani <shaiq.wani@intel.com>
> ---
> drivers/net/intel/common/tx.h | 7 +++
> drivers/net/intel/idpf/idpf_common_rxtx.c | 72 +++++++++++++++--------
> 2 files changed, 55 insertions(+), 24 deletions(-)
>
The AI review linked from patchwork [1] flags some issues with this that
are worth considering.
[1] https://mails.dpdk.org/archives/test-report/2026-September/1052807.html
Running an AI review locally with Claude also reports issues, and some of
them seem quite serious. Can you review this feedback any fix any issues
that are flagged, and let us know if any reports are false positives.
/Bruce
---
Review: net/idpf: fix Tx payload corruption in split queue
The stated fix (shadow ring keyed by compl_tag instead of walking sw_ring[]
by first_id) is a sound idea, but the patch leaves sw_ring[].mbuf populated
during transmit while no longer clearing it on completion. That creates a
serious regression.
Error: double-free / use-after-free of Tx mbufs at queue stop/release (high
confidence) idpf_dp_splitq_xmit_pkts() still writes txe->mbuf = tx_pkt; for
every descriptor (idpf_common_rxtx.c:1048), exactly as before the patch.
But the new IDPF_TXD_COMPLT_RS handler in idpf_split_tx_free() only frees
via txq->tx_pending_pkts[tag] and never touches sw_ring[]/clears txe->mbuf
anymore (idpf_common_rxtx.c:855-863) — previously this path did txe->mbuf =
NULL; after freeing each segment.
Consequently, once a packet's RS completion has been processed normally
during Tx (freeing the mbuf chain through tx_pending_pkts), sw_ring[]
retains a now-dangling pointer to the freed mbuf(s) with no code left to
clear it. On the next queue stop/release:
idpf_tx_queue_stop() (both idpf_rxtx.c:736 and cpfl_rxtx.c:1368) calls
ci_txq_release_all_mbufs(txq, false) before idpf_qc_split_tx_descq_reset().
That function walks sw_ring[last_desc_cleaned+1 .. tx_tail) and calls
rte_pktmbuf_free_seg() on every non-NULL mbuf, i.e. on mbufs that have
already been freed via tx_pending_pkts — a use-after-free/double-free on
already-recycled mempool objects. idpf_qc_tx_queue_release() is worse: it
frees every live entry in tx_pending_pkts[] (idpf_common_rxtx.c:407-416)
and then immediately calls ci_txq_release_all_mbufs(), which frees the very
same still-in-flight (not-yet-RS-completed) mbufs again via their sw_ring[]
entries — an unconditional double-free for any packet in flight at
port-close/queue-release time. This reintroduces memory corruption via a
different path than the one the commit set out to fix, and is now
deterministically triggered by a normal stop/close after any traffic,
rather than only under the original race.
Suggested fix: pick one owner for Tx-mbuf lifetime. Either stop writing
txe->mbuf in the split-queue xmit path (since tx_pending_pkts is now
authoritative) and make ci_txq_release_all_mbufs()/reset aware that split
queues must drain via tx_pending_pkts instead of sw_ring, or keep clearing
txe->mbuf = NULL alongside the tx_pending_pkts free in the RS-completion
handler so the two structures stay in sync.
Error: shadow-ring allocation failure is silently swallowed (medium
confidence) In idpf_qc_split_tx_descq_reset(), if rte_zmalloc_socket() for
tx_pending_pkts fails, the function logs an error and returns, but it is
void and none of its four call sites check for failure
(idpf_common_rxtx.h:217). tx_pending_pkts stays NULL and tx_pending_mask
stays uninitialized, yet idpf_dp_splitq_xmit_pkts() unconditionally does
txq->tx_pending_pkts[tag] = tx_pkts[nb_tx];, which will
NULL-pointer-dereference on the first transmit. This needs to propagate as
a real error (change the function to return int, or fail queue setup/start)
rather than only logging.
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v2] net/intel: fix idpf Tx payload corruption in split queue
2026-09-30 4:46 [PATCH] net/idpf: fix Tx payload corruption in split queue Shaiq Wani
2026-10-06 18:22 ` Bruce Richardson
@ 2026-10-07 10:40 ` Shaiq Wani
1 sibling, 0 replies; 3+ messages in thread
From: Shaiq Wani @ 2026-10-07 10:40 UTC (permalink / raw)
To: dev, bruce.richardson, aman.deep.singh; +Cc: stable
Observed on ACC while running NVMe traffic tests: under sustained
high-throughput split-queue Tx, payload bytes on the wire did not
match what the application submitted, while headers and checksums
looked valid.
The RS-completion path was freeing mbufs by walking sw_ring[] slots
between first_id and the EOP sw_id. sw_ring[] slots get reclaimed
by RE and reused by subsequent submits well before RS lands, so the
walker was freeing mbufs still in flight for later packets, whose
buffers then got recycled and overwritten mid-DMA.
Fix by tracking completion ownership in a shadow ring indexed by a
software-defined compl_tag stamped on all data descriptors of the
packet, as required by the IDPF spec, and echoed back by HW on RS.
The shadow ring is the sole owner of in-flight mbufs on the scalar
split-queue path, so sw_ring[].mbuf is no longer written and queue
stop/release drain only the shadow ring.
Since RE reclaims descriptors before RS completes, nb_tx_free does
not bound the number of packets awaiting RS. Size the shadow ring to
nb_tx_desc and stop transmitting when the next compl_tag slot is
still occupied, so a pending entry is never overwritten and pending
completions cannot overflow the completion queue.
Allocate the shadow ring at queue setup and fail setup on allocation
error, and free it on all setup error and release paths. Remove the
now unused ci_tx_entry.first_id field.
Fixes: 96cc9b6ea60c ("net/idpf: fix multi-segment mbuf leak in split Tx path")
Cc: stable@dpdk.org
Signed-off-by: Shaiq Wani <shaiq.wani@intel.com>
---
v2:
Addressed AI review comments, which include:
* Do not store mbufs in sw_ring[] on the scalar split-queue Tx path;
tx_pending_pkts[] is the sole owner, fixing double-free on queue
stop/release.
* Stop Tx when the next compl_tag slot is still pending, so RS
entries cannot be overwritten when RE runs ahead of RS.
* Allocate the shadow ring at queue setup and propagate -ENOMEM.
* Free the shadow ring on idpf/cpfl setup error paths and in
cpfl_tx_queue_release().
Also aligned with the IDPF spec:
* Write the same compl_tag to all data descriptors of a packet.
* Size the shadow ring to nb_tx_desc, so pending completions cannot
overflow the 2 * nb_tx_desc completion queue.
Other:
* Remove the now unused ci_tx_entry.first_id field.
* Drop redundant per-descriptor sw_ring[].mbuf = NULL stores.
drivers/net/intel/common/tx.h | 8 +-
drivers/net/intel/cpfl/cpfl_rxtx.c | 6 ++
drivers/net/intel/idpf/idpf_common_rxtx.c | 105 ++++++++++++++++------
drivers/net/intel/idpf/idpf_common_rxtx.h | 4 +
drivers/net/intel/idpf/idpf_rxtx.c | 5 ++
5 files changed, 100 insertions(+), 28 deletions(-)
diff --git a/drivers/net/intel/common/tx.h b/drivers/net/intel/common/tx.h
index 2a63cd4330..7c9841a21a 100644
--- a/drivers/net/intel/common/tx.h
+++ b/drivers/net/intel/common/tx.h
@@ -166,7 +166,6 @@ struct ci_tx_queue;
struct ci_tx_entry {
struct rte_mbuf *mbuf; /* mbuf associated with TX desc, if any. */
uint16_t next_id; /* Index of next descriptor in ring. */
- uint16_t first_id; /* Split-queue: first sw_id of packet at EOP entry. */
};
/**
@@ -262,6 +261,13 @@ struct ci_tx_queue {
uint16_t sw_nb_desc;
uint16_t sw_tail;
uint16_t rs_compl_count;
+ /* Split-Tx completion tracking: shadow ring indexed by a
+ * rolling compl_tag decouples RS-completion mbuf lifetime
+ * from sw_ring[] slot reuse. Sized nb_tx_desc.
+ */
+ struct rte_mbuf **tx_pending_pkts;
+ uint16_t tx_pending_size;
+ uint16_t tx_next_compl_tag;
};
};
};
diff --git a/drivers/net/intel/cpfl/cpfl_rxtx.c b/drivers/net/intel/cpfl/cpfl_rxtx.c
index aba3a5e916..0dbe47d336 100644
--- a/drivers/net/intel/cpfl/cpfl_rxtx.c
+++ b/drivers/net/intel/cpfl/cpfl_rxtx.c
@@ -329,6 +329,7 @@ cpfl_tx_queue_release(void *txq)
rte_free(q->complq);
}
+ idpf_qc_split_tx_pending_free(q);
ci_txq_release_all_mbufs(q, q->use_vec_entry);
rte_free(q->sw_ring);
rte_free(q->rs_last_id);
@@ -624,6 +625,9 @@ cpfl_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
idpf_qc_single_tx_queue_reset(txq);
} else {
txq->desc_ring = mz->addr;
+ ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
+ if (ret != 0)
+ goto err_pending_alloc;
idpf_qc_split_tx_descq_reset(txq);
/* Setup tx completion queue if split model */
@@ -642,6 +646,8 @@ cpfl_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
return 0;
err_complq_setup:
+ idpf_qc_split_tx_pending_free(txq);
+err_pending_alloc:
rte_free(txq->rs_last_id);
err_rs_last_id_alloc:
rte_free(txq->sw_ring);
diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.c b/drivers/net/intel/idpf/idpf_common_rxtx.c
index 649b5d1f99..e54056b5c0 100644
--- a/drivers/net/intel/idpf/idpf_common_rxtx.c
+++ b/drivers/net/intel/idpf/idpf_common_rxtx.c
@@ -233,6 +233,51 @@ idpf_qc_split_tx_descq_reset(struct ci_tx_queue *txq)
txq->rs_compl_count = 0;
txq->tx_next_dd = txq->tx_rs_thresh - 1;
txq->tx_next_rs = txq->tx_rs_thresh - 1;
+
+ if (txq->tx_pending_pkts != NULL) {
+ for (i = 0; i < txq->tx_pending_size; i++) {
+ if (txq->tx_pending_pkts[i] != NULL) {
+ rte_pktmbuf_free(txq->tx_pending_pkts[i]);
+ txq->tx_pending_pkts[i] = NULL;
+ }
+ }
+ }
+ txq->tx_next_compl_tag = 0;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(idpf_qc_split_tx_pending_alloc)
+int
+idpf_qc_split_tx_pending_alloc(struct ci_tx_queue *txq, unsigned int socket_id)
+{
+ /* Bounding pending RS to nb_tx_desc keeps the 2 * nb_tx_desc complq from overflowing. */
+ txq->tx_pending_pkts = rte_zmalloc_socket("idpf_tx_pending",
+ sizeof(struct rte_mbuf *) * txq->nb_tx_desc,
+ RTE_CACHE_LINE_SIZE, socket_id);
+ if (txq->tx_pending_pkts == NULL) {
+ DRV_LOG(ERR, "Failed to alloc idpf tx_pending shadow ring");
+ return -ENOMEM;
+ }
+ txq->tx_pending_size = txq->nb_tx_desc;
+ txq->tx_next_compl_tag = 0;
+
+ return 0;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(idpf_qc_split_tx_pending_free)
+void
+idpf_qc_split_tx_pending_free(struct ci_tx_queue *txq)
+{
+ uint32_t i;
+
+ if (txq->tx_pending_pkts == NULL)
+ return;
+
+ for (i = 0; i < txq->tx_pending_size; i++) {
+ if (txq->tx_pending_pkts[i] != NULL)
+ rte_pktmbuf_free(txq->tx_pending_pkts[i]);
+ }
+ rte_free(txq->tx_pending_pkts);
+ txq->tx_pending_pkts = NULL;
}
RTE_EXPORT_INTERNAL_SYMBOL(idpf_qc_split_tx_complq_reset)
@@ -387,6 +432,7 @@ idpf_qc_tx_queue_release(void *txq)
rte_free(q->complq);
}
+ idpf_qc_split_tx_pending_free(q);
ci_txq_release_all_mbufs(q, false);
rte_free(q->rs_last_id);
rte_free(q->sw_ring);
@@ -787,7 +833,6 @@ idpf_split_tx_free(struct idpf_complq *cq)
volatile struct idpf_splitq_tx_compl_desc *compl_ring = cq->compl_ring;
volatile struct idpf_splitq_tx_compl_desc *txd;
uint16_t next = cq->tx_tail;
- struct ci_tx_entry *txe;
struct ci_tx_queue *txq;
uint16_t gen, qid, q_head;
uint16_t nb_desc_clean;
@@ -824,26 +869,21 @@ idpf_split_tx_free(struct idpf_complq *cq)
txq->nb_tx_free += nb_desc_clean;
txq->last_desc_cleaned = q_head;
break;
- case IDPF_TXD_COMPLT_RS:
- /* Walk from first segment to EOP, freeing each segment. */
- txe = &txq->sw_ring[q_head];
- if (txe->mbuf != NULL) {
- uint16_t first = txe->first_id;
- uint16_t idx = first;
- uint16_t end = (q_head + 1 == txq->sw_nb_desc) ?
- 0 : q_head + 1;
-
- do {
- txe = &txq->sw_ring[idx];
- if (txe->mbuf != NULL) {
- rte_pktmbuf_free_seg(txe->mbuf);
- txe->mbuf = NULL;
- }
- idx = (idx + 1 == txq->sw_nb_desc) ?
- 0 : idx + 1;
- } while (idx != end);
+ case IDPF_TXD_COMPLT_RS: {
+ /* Shadow ring indexed by the software-defined compl_tag is
+ * the sole source of truth for RS-time mbuf ownership; free
+ * the whole multi-seg chain via mbuf->next in one call.
+ */
+ uint16_t tag = q_head;
+
+ if (unlikely(tag >= txq->tx_pending_size)) {
+ TX_LOG(ERR, "invalid completion tag %u.", tag);
+ } else if (txq->tx_pending_pkts[tag] != NULL) {
+ rte_pktmbuf_free(txq->tx_pending_pkts[tag]);
+ txq->tx_pending_pkts[tag] = NULL;
}
break;
+ }
default:
TX_LOG(ERR, "unknown completion type.");
return;
@@ -976,6 +1016,19 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
if (txq->nb_tx_free < nb_used)
break;
+ /* RE reclaims descriptors before RS, so tag occupancy is the
+ * only bound on packets awaiting RS completion.
+ */
+ uint16_t tag = txq->tx_next_compl_tag;
+
+ if (unlikely(txq->tx_pending_pkts[tag] != NULL)) {
+ nb_to_clean = 2 * txq->tx_rs_thresh;
+ while (nb_to_clean--)
+ idpf_split_tx_free(txq->complq);
+ if (txq->tx_pending_pkts[tag] != NULL)
+ break;
+ }
+
if (ol_flags & CI_TX_CKSUM_OFFLOAD_MASK)
cmd_dtype = IDPF_TXD_FLEX_FLOW_CMD_CS_EN;
@@ -991,8 +1044,6 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
tx_id = 0;
}
- uint16_t first_sw_id = sw_id;
-
do {
uint16_t slen = tx_pkt->data_len;
rte_iova_t buf_dma_addr = rte_mbuf_data_iova(tx_pkt);
@@ -1004,13 +1055,12 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
unlikely(slen > CI_MAX_DATA_PER_TXD)) {
txd = &txr[tx_id];
txn = &sw_ring[txe->next_id];
- txe->mbuf = NULL;
txd->buf_addr = rte_cpu_to_le_64(buf_dma_addr);
txd->qw1.cmd_dtype = cmd_dtype |
IDPF_TX_DESC_DTYPE_FLEX_FLOW_SCHE;
txd->qw1.rxr_bufsize = CI_MAX_DATA_PER_TXD;
- txd->qw1.compl_tag = sw_id;
+ txd->qw1.compl_tag = tag;
buf_dma_addr += CI_MAX_DATA_PER_TXD;
slen -= CI_MAX_DATA_PER_TXD;
@@ -1024,14 +1074,14 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
txd = &txr[tx_id];
txn = &sw_ring[txe->next_id];
- txe->mbuf = tx_pkt;
/* Setup TX descriptor */
txd->buf_addr = rte_cpu_to_le_64(buf_dma_addr);
txd->qw1.cmd_dtype = cmd_dtype |
IDPF_TX_DESC_DTYPE_FLEX_FLOW_SCHE;
txd->qw1.rxr_bufsize = slen;
- txd->qw1.compl_tag = sw_id;
+ /* Spec: COMPLETION_TAG must be identical on all data descs of a packet. */
+ txd->qw1.compl_tag = tag;
tx_id++;
if (tx_id == txq->nb_tx_desc)
tx_id = 0;
@@ -1043,8 +1093,9 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
/* fill the last descriptor with End of Packet (EOP) bit */
txd->qw1.cmd_dtype |= IDPF_TXD_FLEX_FLOW_CMD_EOP;
- /* Record first sw_id at EOP so completion can walk forward. */
- sw_ring[txd->qw1.compl_tag].first_id = first_sw_id;
+ txq->tx_pending_pkts[tag] = tx_pkts[nb_tx];
+ if (++txq->tx_next_compl_tag == txq->tx_pending_size)
+ txq->tx_next_compl_tag = 0;
txq->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_used);
txq->rs_compl_count += nb_used;
diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.h b/drivers/net/intel/idpf/idpf_common_rxtx.h
index b2d33287df..3365631d80 100644
--- a/drivers/net/intel/idpf/idpf_common_rxtx.h
+++ b/drivers/net/intel/idpf/idpf_common_rxtx.h
@@ -216,6 +216,10 @@ void idpf_qc_single_rx_queue_reset(struct idpf_rx_queue *rxq);
__rte_internal
void idpf_qc_split_tx_descq_reset(struct ci_tx_queue *txq);
__rte_internal
+int idpf_qc_split_tx_pending_alloc(struct ci_tx_queue *txq, unsigned int socket_id);
+__rte_internal
+void idpf_qc_split_tx_pending_free(struct ci_tx_queue *txq);
+__rte_internal
void idpf_qc_split_tx_complq_reset(struct idpf_complq *cq);
__rte_internal
void idpf_splitq_rearm_common(struct idpf_rx_queue *rx_bufq);
diff --git a/drivers/net/intel/idpf/idpf_rxtx.c b/drivers/net/intel/idpf/idpf_rxtx.c
index 077a92a8a9..0d41cd87eb 100644
--- a/drivers/net/intel/idpf/idpf_rxtx.c
+++ b/drivers/net/intel/idpf/idpf_rxtx.c
@@ -495,6 +495,9 @@ idpf_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
idpf_qc_single_tx_queue_reset(txq);
} else {
txq->desc_ring = mz->addr;
+ ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
+ if (ret != 0)
+ goto err_pending_alloc;
idpf_qc_split_tx_descq_reset(txq);
/* Setup tx completion queue if split model */
@@ -512,6 +515,8 @@ idpf_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
return 0;
err_complq_setup:
+ idpf_qc_split_tx_pending_free(txq);
+err_pending_alloc:
rte_free(txq->rs_last_id);
err_rs_last_id_alloc:
rte_free(txq->sw_ring);
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread