Re: [PATCH net] idpf: free the skb on TX DMA mapping errors

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves the skb leak in place for HW-timestamped packets,
  and the Tx timestamp latch they hold is also never released.

Pre-existing issues:
- [Medium] This bug was not introduced by this patch, but in FLOW_SCH_EN
  mode idpf_tx_splitq_frame() increments…

--

idpf: free the skb on TX DMA mapping errors

This commit frees the skb at the end of the TX DMA mapping error unwind
in idpf_tx_splitq_pkt_err_unmap() and idpf_tx_singleq_dma_map_error().
At that point the first buffer is not yet typed LIBETH_SQE_SKB, so
libeth_tx_complete() only unmaps it, and before this change the skb
leaked.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 885df6294ad94..a92e2457781e6 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -2606,6 +2606,7 @@ static void idpf_tx_splitq_pkt_err_unmap(struct idpf_tx_queue *txq,
>  	struct idpf_sw_queue *refillq = txq->refillq;
>  	struct libeth_sq_napi_stats ss = { };
>  	struct idpf_tx_buf *tx_buf = first;
> +	struct sk_buff *skb = first->skb;
>  	struct libeth_cq_pp cp = {
>  		.dev    = txq->dev,
>  		.ss     = &ss,
> @@ -2621,6 +2622,8 @@ static void idpf_tx_splitq_pkt_err_unmap(struct idpf_tx_queue *txq,
>  		libeth_tx_complete(tx_buf, &cp);
>  	}
>  
> +	dev_kfree_skb_any(skb);

[Severity: High]
Does this still leak the skb for packets that asked for a HW Tx
timestamp?

idpf_tx_splitq_frame() reserves a timestamp latch before it maps the
packet:

idpf_tx_splitq_frame() {
    ...
	idx = idpf_tx_tstamp(tx_q, skb, &tx_params.offload);
	if (idx != -1) {
		ctx_desc = idpf_tx_splitq_get_ctx_desc(tx_q);
		idpf_tx_set_tstamp_desc(ctx_desc, idx);
	}
    ...
	idpf_tx_splitq_map(tx_q, &tx_params, first);
}

idpf_tx_tstamp()->idpf_ptp_request_ts() then takes a second reference
on the skb and moves the latch to latches_in_use:

idpf_ptp_request_ts() {
    ...
	ptp_tx_tstamp->skb = skb_get(skb);
	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;

	/* Move the element to the used latches list */
	list_add(&ptp_tx_tstamp->list_member,
		 &tx_q->cached_tstamp_caps->latches_in_use);
    ...
}

idpf_tx_splitq_map() can then fail in two ways: dma_mapping_error(),
or idpf_tx_get_free_buf_id() failing between fragments in FLOW_SCH_EN
mode. In either case this dev_kfree_skb_any() drops only the xmit
reference. The tail is rewound to params->prev_ntu, so the hardware
never fetches the TSYN context descriptor.

Normally a latch is retired only through the completion path.
idpf_tx_clean_bufs() sees a LIBETH_SQE_SKB buffer with
SKBTX_IN_PROGRESS set and calls idpf_tx_read_tstamp(). After that,
idpf_ptp_get_tx_tstamp() asks firmware about latches in the
IDPF_PTP_REQUEST state.

A packet that went through this unwind never reaches that path. The
only other cleanup is idpf_ptp_release_vport_tstamp() at vport
teardown.

Wouldn't the skb then stay pinned until teardown, along with the socket
wmem charged to it, and one hardware latch per failure? Once every latch
is used up this way, it looks like all later HW Tx timestamp requests
would fail with -ENOBUFS until the vport is torn down.

The refillq exhaustion drop path in idpf_tx_splitq_frame() also runs
after the latch has been reserved. It seems to be missing the same
rollback:

	if (unlikely(!idpf_tx_get_free_buf_id(tx_q->refillq,
					      &buf_id))) {
		...
		tx_q->next_to_use = tx_params.prev_ntu;
		return idpf_tx_drop_skb(tx_q, skb);
	}

Should the unwind also move the latch back to latches_free, clear
SKBTX_IN_PROGRESS, and drop the latch's skb reference?

> +
>  	/* Update tail in case netdev_xmit_more was previously true. */
>  	idpf_tx_buf_hw_update(txq, params->prev_ntu, false);

[Severity: Medium]
This is a pre-existing issue that was not introduced by this patch, but
does this unwind leave num_completions_pending too high?

In FLOW_SCH_EN mode, idpf_tx_splitq_frame() increments the counter and
sets last_re before calling idpf_tx_splitq_map():

	if (idpf_tx_splitq_need_re(tx_q)) {
		tx_params.eop_cmd |= IDPF_TXD_FLEX_FLOW_CMD_RE;
		tx_q->txq_grp->num_completions_pending++;
		tx_q->last_re = tx_q->next_to_use;
	}

When mapping fails, the tail is rewound to prev_ntu. The RE descriptor
is never posted, so its completion never arrives. This unwind restores
neither num_completions_pending nor last_re.

When a context descriptor was used, the stale last_re can also make
idpf_tx_splitq_need_re() return true again for the retried packet.

Once IDPF_TX_COMPLQ_PENDING() exceeds IDPF_TX_COMPLQ_OVERFLOW_THRESH(),
idpf_txq_has_room() returns 0:

	if (IDPF_DESC_UNUSED(tx_q) < descs_needed ||
	    IDPF_TX_COMPLQ_PENDING(tx_q->txq_grp) >
		IDPF_TX_COMPLQ_OVERFLOW_THRESH(tx_q->txq_grp->complq) ||
	...

idpf_tx_clean_complq() also sets complq_ok = false, so the queue is
never woken.

Could enough of these failures stall the Tx queue until the tx timeout
reset reinitializes the counter?

>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024030401.852598-1-almasrymina%40google.com