* Re: [PATCH net] idpf: free the skb on TX DMA mapping errors
2026-10-24 3:04 [PATCH net] idpf: free the skb on TX DMA mapping errors Mina Almasry
@ 2026-10-24 3:05 ` netdev-bot+sinfo
2026-10-24 3:09 ` Mina Almasry
2026-10-25 3:04 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: netdev-bot+sinfo @ 2026-10-24 3:05 UTC (permalink / raw)
To: Mina Almasry
Cc: Alexander Lobakin, Tony Nguyen, Przemek Kitszel, netdev,
linux-kernel, Naman Gulati, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] idpf: free the skb on TX DMA mapping errors
2026-10-24 3:04 [PATCH net] idpf: free the skb on TX DMA mapping errors Mina Almasry
2026-10-24 3:05 ` netdev-bot+sinfo
@ 2026-10-25 3:04 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 3:04 UTC (permalink / raw)
To: almasrymina
Cc: aleksander.lobakin, anthony.l.nguyen, przemyslaw.kitszel, netdev,
linux-kernel, namangulati, andrew+netdev, davem, edumazet, kuba,
pabeni
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
^ permalink raw reply [flat|nested] 4+ messages in thread