Re: [PATCH] net/idpf: fix Tx payload corruption in split queue

[Bruce Richardson <bruce.richardson@intel.com>]

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.