Re: [PATCH v1] net/intel: fix mbuf leak in idpf split queue Tx path

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

On Thu, Mar 12, 2026 at 04:31:33PM +0530, Shaiq Wani wrote:
> RS completion only frees the EOP entry's mbuf, leaking preceding
> segments of multi-segment packets. Add a first_id field to the Tx
> entry struct, record the first sw_id at the EOP entry during submit,
> and walk forward from first to EOP to free all segments on completion.
> 
> Fixes: 8c6098afa075 ("common/idpf: add Rx/Tx data path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shaiq Wani <shaiq.wani@intel.com>
> ---
>  drivers/net/intel/common/tx.h             |  1 +
>  drivers/net/intel/idpf/idpf_common_rxtx.c | 26 ++++++++++++++++++++---
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/intel/common/tx.h b/drivers/net/intel/common/tx.h
> index 283bd58d5d..53e61c0b62 100644
> --- a/drivers/net/intel/common/tx.h
> +++ b/drivers/net/intel/common/tx.h
> @@ -121,6 +121,7 @@ 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. */
>  };
>  
>  /**
> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.c b/drivers/net/intel/idpf/idpf_common_rxtx.c
> index f73716e57c..bc0677f2ee 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx.c
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx.c
> @@ -825,11 +825,23 @@ idpf_split_tx_free(struct ci_tx_queue *cq)
>  		txq->last_desc_cleaned = q_head;
>  		break;
>  	case IDPF_TXD_COMPLT_RS:
> -		/* q_head indicates sw_id when ctype is 2 */
> +		/* Walk from first segment to EOP, freeing each segment. */
>  		txe = &txq->sw_ring[q_head];
>  		if (txe->mbuf != NULL) {
> -			rte_pktmbuf_free_seg(txe->mbuf);
> -			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);
>  		}
>  		break;
>  	default:
> @@ -979,9 +991,14 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  				tx_id = 0;
>  		}
>  
> +		uint16_t first_sw_id = sw_id;
> +
>  		do {
>  			txd = &txr[tx_id];
>  			txn = &sw_ring[txe->next_id];
> +
> +			if (txe->mbuf != NULL)
> +				rte_pktmbuf_free_seg(txe->mbuf);

Does hitting this point not indicate a problem condition? If
idpf_split_tx_free has not cleaned this slot, surely it's not correct for
us to free a potentially unsent mbuf?

Also, does this code path use out-of-order completions? If it does, then I
think we need to always check each slot for NULL before we use it, and stop
as soon as we hit a non-null descriptor (potentially undoing any descriptor
writes we have already done for the current packet).

>  			txe->mbuf = tx_pkt;
>  
>  			/* Setup TX descriptor */
> @@ -1002,6 +1019,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->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_used);
>  		txq->rs_compl_count += nb_used;
>  
> -- 
> 2.34.1
>