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

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

[Vincent Choiniere via B4 Relay]

From: Vincent Choiniere <choiniere.vincent@hydroquebec.com>

ice_tx_hang_reporter_dump() always reads tx_ring->tx_buf[0].skb,
regardless of next_to_clean. The Tx cleanup path frees the skb and
marks the buffer empty without clearing the skb pointer, so slot 0 can
hold a stale pointer. A NULL check alone does not cover this, and
devlink_fmsg_dump_skb() can fault on the freed skb.

Read tx_buf[next_to_clean] instead, use its skb only if the buffer type
is ICE_TX_BUF_SKB, and skip the skb dump otherwise.

Observed on an E810 PF passed through to a KVM guest (Ubuntu 7.0.0-14,
PREEMPT_RT, local scheduler patch): a delayed queue IRQ led to a Tx
timeout and a page fault in devlink_fmsg_dump_skb():

 BUG: unable to handle page fault for address: 0000000004cd0c11
 Workqueue: ice ice_service_task [ice]
 RIP: devlink_fmsg_dump_skb+0x2d8/0x5d0
 Call Trace:
  ice_tx_hang_reporter_dump+0x1ce/0x210 [ice]
  devlink_health_do_dump+0xcc/0x190
  devlink_health_report+0x1d7/0x260
  ice_report_tx_hang+0x7d/0xa0 [ice]

With this change and auto_dump enabled, starving a queue IRQ thread
reproduced the Tx timeout; the dump contained the test packet, no Oops
occurred and the PF recovered.

Fixes: 2a82874a3b7b ("ice: add Tx hang devlink health reporter")
Link: https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/
Cc: stable@vger.kernel.org
Signed-off-by: Vincent Choiniere <choiniere.vincent@hydroquebec.com>
---
This complements the NULL check in [1]. With only that check, a non-NULL
but stale tx_buf[0].skb is still dereferenced by devlink_fmsg_dump_skb().
I have only reproduced the crash on an Ubuntu 7.0.0-14 kernel with
PREEMPT_RT and a local scheduler patch, and the skb lifetime against
concurrent Tx cleanup still needs review. Feedback welcome.

In my case next_to_clean was 0xfc while the reporter read slot 0.
Upstream v7.0, v7.2 and current net main (191aa4154) all read
tx_buf[0].skb unconditionally. Before this change, recovery stayed
stuck until reboot.

[1] https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/
---
 drivers/net/ethernet/intel/ice/devlink/health.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
index 8e9a8a817..059859c36 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,
 				     struct netlink_ext_ack *extack)
 {
 	struct ice_tx_hang_event *event = priv_ctx;
-	struct sk_buff *skb;
+	struct sk_buff *skb = NULL;
+	struct ice_tx_buf *tx_buf;
 
 	if (!event)
 		return 0;
 
-	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;
 	devlink_fmsg_obj_nest_start(fmsg);
 	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
 	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, intr);
@@ -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));
-	devlink_fmsg_dump_skb(fmsg, skb);
+	if (skb)
+		devlink_fmsg_dump_skb(fmsg, skb);
 	devlink_fmsg_obj_nest_end(fmsg);
 
 	return 0;

---
base-commit: 191aa41548b2d2b6af534a6d5cf84a552435c74c
change-id: 20261016-ice-tx-hang-dump-e46b4ed483d6

Best regards,
--  
Vincent Choiniere <choiniere.vincent@hydroquebec.com>

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

[Paul Menzel]

Dear Vincent,


Thank you for your patch.

Am 16.10.26 um 15:08 schrieb Vincent Choiniere via B4 Relay:
> From: Vincent Choiniere <choiniere.vincent@hydroquebec.com>
> 
> ice_tx_hang_reporter_dump() always reads tx_ring->tx_buf[0].skb,
> regardless of next_to_clean. The Tx cleanup path frees the skb and
> marks the buffer empty without clearing the skb pointer, so slot 0 can
> hold a stale pointer. A NULL check alone does not cover this, and
> devlink_fmsg_dump_skb() can fault on the freed skb.
> 
> Read tx_buf[next_to_clean] instead, use its skb only if the buffer type
> is ICE_TX_BUF_SKB, and skip the skb dump otherwise.
> 
> Observed on an E810 PF passed through to a KVM guest (Ubuntu 7.0.0-14,
> PREEMPT_RT, local scheduler patch): a delayed queue IRQ led to a Tx
> timeout and a page fault in devlink_fmsg_dump_skb():
> 
>   BUG: unable to handle page fault for address: 0000000004cd0c11
>   Workqueue: ice ice_service_task [ice]
>   RIP: devlink_fmsg_dump_skb+0x2d8/0x5d0
>   Call Trace:
>    ice_tx_hang_reporter_dump+0x1ce/0x210 [ice]
>    devlink_health_do_dump+0xcc/0x190
>    devlink_health_report+0x1d7/0x260
>    ice_report_tx_hang+0x7d/0xa0 [ice]
> 
> With this change and auto_dump enabled, starving a queue IRQ thread
> reproduced the Tx timeout; the dump contained the test packet, no Oops
> occurred and the PF recovered.

Also past the dump (excerpt)?

> Fixes: 2a82874a3b7b ("ice: add Tx hang devlink health reporter")
> Link: https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Vincent Choiniere <choiniere.vincent@hydroquebec.com>
> ---
> This complements the NULL check in [1]. With only that check, a non-NULL
> but stale tx_buf[0].skb is still dereferenced by devlink_fmsg_dump_skb().
> I have only reproduced the crash on an Ubuntu 7.0.0-14 kernel with
> PREEMPT_RT and a local scheduler patch, and the skb lifetime against
> concurrent Tx cleanup still needs review. Feedback welcome.
> 
> In my case next_to_clean was 0xfc while the reporter read slot 0.
> Upstream v7.0, v7.2 and current net main (191aa4154) all read
> tx_buf[0].skb unconditionally. Before this change, recovery stayed
> stuck until reboot.
> 
> [1] https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/
> ---
>   drivers/net/ethernet/intel/ice/devlink/health.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
> index 8e9a8a817..059859c36 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,
>   				     struct netlink_ext_ack *extack)
>   {
>   	struct ice_tx_hang_event *event = priv_ctx;
> -	struct sk_buff *skb;
> +	struct sk_buff *skb = NULL;
> +	struct ice_tx_buf *tx_buf;
>   
>   	if (!event)
>   		return 0;
>   
> -	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;
>   	devlink_fmsg_obj_nest_start(fmsg);
>   	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
>   	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, intr);
> @@ -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));
> -	devlink_fmsg_dump_skb(fmsg, skb);
> +	if (skb)
> +		devlink_fmsg_dump_skb(fmsg, skb);
>   	devlink_fmsg_obj_nest_end(fmsg);
>   
>   	return 0;
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

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

[Przemek Kitszel]

On 10/16/26 15:08, Vincent Choiniere via B4 Relay wrote:
> From: Vincent Choiniere <choiniere.vincent@hydroquebec.com>
> 
> ice_tx_hang_reporter_dump() always reads tx_ring->tx_buf[0].skb,
> regardless of next_to_clean. The Tx cleanup path frees the skb and
> marks the buffer empty without clearing the skb pointer, so slot 0 can
> hold a stale pointer. A NULL check alone does not cover this, and
> devlink_fmsg_dump_skb() can fault on the freed skb.
> 
> Read tx_buf[next_to_clean] instead, use its skb only if the buffer type
> is ICE_TX_BUF_SKB, and skip the skb dump otherwise.
> 
> Observed on an E810 PF passed through to a KVM guest (Ubuntu 7.0.0-14,
> PREEMPT_RT, local scheduler patch): a delayed queue IRQ led to a Tx
> timeout and a page fault in devlink_fmsg_dump_skb():
> 
>   BUG: unable to handle page fault for address: 0000000004cd0c11
>   Workqueue: ice ice_service_task [ice]
>   RIP: devlink_fmsg_dump_skb+0x2d8/0x5d0
>   Call Trace:
>    ice_tx_hang_reporter_dump+0x1ce/0x210 [ice]
>    devlink_health_do_dump+0xcc/0x190
>    devlink_health_report+0x1d7/0x260
>    ice_report_tx_hang+0x7d/0xa0 [ice]
> 
> With this change and auto_dump enabled, starving a queue IRQ thread
> reproduced the Tx timeout; the dump contained the test packet, no Oops
> occurred and the PF recovered.
> 
> Fixes: 2a82874a3b7b ("ice: add Tx hang devlink health reporter")
> Link: https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Vincent Choiniere <choiniere.vincent@hydroquebec.com>
> ---
> This complements the NULL check in [1]. With only that check, a non-NULL
> but stale tx_buf[0].skb is still dereferenced by devlink_fmsg_dump_skb().
> I have only reproduced the crash on an Ubuntu 7.0.0-14 kernel with
> PREEMPT_RT and a local scheduler patch, and the skb lifetime against
> concurrent Tx cleanup still needs review. Feedback welcome.
> 
> In my case next_to_clean was 0xfc while the reporter read slot 0.
> Upstream v7.0, v7.2 and current net main (191aa4154) all read
> tx_buf[0].skb unconditionally. Before this change, recovery stayed
> stuck until reboot.
> 
> [1] https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/

thank you, looks better than simple null check
please add some more null checks as asked by sashiko, then:

Acked-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>


> ---
>   drivers/net/ethernet/intel/ice/devlink/health.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
> index 8e9a8a817..059859c36 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,
>   				     struct netlink_ext_ack *extack)
>   {
>   	struct ice_tx_hang_event *event = priv_ctx;
> -	struct sk_buff *skb;
> +	struct sk_buff *skb = NULL;
> +	struct ice_tx_buf *tx_buf;
>   
>   	if (!event)
>   		return 0;
>   
> -	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;
>   	devlink_fmsg_obj_nest_start(fmsg);
>   	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
>   	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, intr);
> @@ -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));
> -	devlink_fmsg_dump_skb(fmsg, skb);
> +	if (skb)
> +		devlink_fmsg_dump_skb(fmsg, skb);
>   	devlink_fmsg_obj_nest_end(fmsg);
>   
>   	return 0;
> 
> ---
> base-commit: 191aa41548b2d2b6af534a6d5cf84a552435c74c
> change-id: 20261016-ice-tx-hang-dump-e46b4ed483d6
> 
> Best regards,
> --
> Vincent Choiniere <choiniere.vincent@hydroquebec.com>
> 
> 

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

[netdev-bot+sashiko]

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