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

[Naman Gulati]

On Sat, Oct 24, 2026 at 8:04 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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.

Both issues are pre-existing. This patch does not introduce any leaks
and these should handled separately in the tx tstamp and FLOW_SCH_EN
code paths.

>
> 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