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