Re: [PATCH net] ice: fix Tx hang dump using stale tx_buf[0].skb

[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 new `tx_buf->type == ICE_TX_BUF_SKB` guard in
  ice_tx_hang_reporter_dump()…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  keeps it and adds another access to the same storage.

--

ice: fix Tx hang dump using stale tx_buf[0].skb

This commit changes ice_tx_hang_reporter_dump() so it reads the Tx buffer
at event->next_to_clean instead of always reading tx_buf[0]. It uses that
buffer's skb only when the buffer type is ICE_TX_BUF_SKB, and skips the skb
dump otherwise.

Link: https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/

> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
> index 8e9a8a8178d48..059859c36dfb2 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c
> @@ -390,12 +390,15 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,

[ ... ]

> -	skb = event->tx_ring->tx_buf->skb;
> +	tx_buf = &event->tx_ring->tx_buf[event->next_to_clean];
> +	if (tx_buf->type == ICE_TX_BUF_SKB)
> +		skb = tx_buf->skb;

[Severity: High]
Is this type check enough to stop devlink_fmsg_dump_skb() from reading a
freed skb when Tx cleanup runs at the same time?

The dump runs from the service task:

ice_service_task()
  ice_report_tx_hang()
    devlink_health_report()
      devlink_health_do_dump()
        ice_tx_hang_reporter_dump()

The only lock held on that path is reporter->dump_lock. Nothing disables
NAPI, takes the Tx queue lock or takes a reference on the skb.

ice_service_task() calls ice_report_tx_hang() before ice_reset_subtask(),
so the ring's NAPI and queue IRQ are still live while the dump runs:

	if (pf->health_reporters.tx_hang_buf.tx_ring) {
		ice_report_tx_hang(pf);
		pf->health_reporters.tx_hang_buf.tx_ring = NULL;
	}

	ice_reset_subtask(pf);

In ice_clean_tx_irq(), the skb is freed before the type is changed, and
tx_buf->skb is never cleared:

		/* free the skb */
		napi_consume_skb(tx_buf->skb, napi_budget);

		/* unmap skb header data */
		dma_unmap_single(tx_ring->dev,
		...
		/* clear tx_buf data */
		tx_buf->type = ICE_TX_BUF_EMPTY;

This means the reporter can see ICE_TX_BUF_SKB for an skb that has already
been freed.

The index is racy as well. ice_report_tx_hang() copies
tx_ring->next_to_clean with a plain read of a field that NAPI updates, so
it can name the slot NAPI is reclaiming at that moment.

After this check and before devlink_fmsg_dump_skb(), the dump makes many
devlink_fmsg_put() calls that allocate with GFP_KERNEL. It also copies the
whole descriptor ring. That is a long window in which the task can be
preempted.

The scenario in the commit message seems to set up exactly this case. The
descriptors have completed and only the queue IRQ is delayed. NAPI can run
on another CPU, as the RT IRQ thread preempting the kworker, or because Rx
traffic on the shared q_vector kicks it. Whenever it runs, it frees the
skb at next_to_clean. devlink_fmsg_dump_skb() would then read skb->len,
skb_shinfo(skb) and skb->sk from freed memory.

Could that cause the same page fault this patch is meant to fix, or copy
freed memory into the dump? The notes below the commit message already say
"the skb lifetime against concurrent Tx cleanup still needs review".

Would it work to call napi_disable() and napi_enable() on
tx_ring->q_vector->napi around reading and dumping the skb? Two other
options are to capture the skb fields while cleanup is excluded, or to
dump only the skb pointer and not the skb contents.

> @@ -409,7 +412,8 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
>  	ice_fmsg_put_ptr(fmsg, "skb-ptr", skb);
>  	devlink_fmsg_binary_pair_put(fmsg, "desc", event->tx_ring->desc,
>  				     event->tx_ring->count * sizeof(struct ice_tx_desc));

[Severity: Medium]
This isn't a bug introduced by this patch, but the patch keeps it and adds
one more access through the same saved ring pointer. Is event->tx_ring
guaranteed to still be valid when this dump runs?

ice_prep_tx_hang_report() saves a raw ring pointer from watchdog context:

	buf->tx_ring = tx_ring;

ice_service_task() uses it later with no lock and no reference. It holds
neither rtnl nor ICE_CFG_BUSY, and ring teardown never clears the saved
pointer.

ice_set_ringparam() runs under rtnl and does not check
ice_is_reset_in_progress(). It calls ice_down(), which runs
ice_clean_tx_ring(). That frees every skb, clears tx_buf and resets
next_to_clean to 0. Then ice_set_ringparam() does:

			ice_for_each_txq(vsi, i) {
				ice_free_tx_ring(vsi->tx_rings[i]);
				*vsi->tx_rings[i] = tx_rings[i];
			}

ice_free_tx_ring() frees tx_buf and desc and sets both to NULL.

If ethtool -G runs while the service task is dumping, can
ice_tx_hang_reporter_dump() do any of the following?

- Index freed or NULL tx_buf memory using the old next_to_clean.
- Dump count * sizeof(struct ice_tx_desc) bytes from a freed or NULL desc.
  The count and desc values may also come from different ring
  generations.
- Dereference q_vector->name.

On VSI rebuild, ice_vsi_clear_rings() frees the ring structs themselves
with kfree_rcu(). That would leave the saved pointer completely stale.

ice_stop() is mostly blocked here. ice_tx_timeout() sets ICE_PFR_REQ, and
ice_stop() returns -EBUSY while a reset is in progress.

The tx_hang_buf fields are also written and read with plain accesses.
Because ice_service_task() clears tx_ring after reporting, it can drop a
newer timeout record. The head and intr values may also come from a
different event than tx_ring.

The old code had the same exposure through
event->tx_ring->tx_buf->skb.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-ice-tx-hang-dump-v1-1-3df0f5979a51%40hydroquebec.com