* [PATCH net] ice: fix Tx hang dump using stale tx_buf[0].skb
@ 2026-10-16 13:08 ` Vincent Choiniere
0 siblings, 0 replies; 4+ messages in thread
From: Vincent Choiniere @ 2026-10-16 13:08 UTC (permalink / raw)
To: Tony Nguyen, Przemek Kitszel, intel-wired-lan
Cc: Aleksandr Loktionov, Michal Kubiak, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
stable, Vincent Choiniere
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>
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] ice: fix Tx hang dump using stale tx_buf[0].skb
2026-10-16 13:08 ` Vincent Choiniere
(?)
@ 2026-10-16 16:56 ` Paul Menzel
-1 siblings, 0 replies; 4+ messages in thread
From: Paul Menzel @ 2026-10-16 16:56 UTC (permalink / raw)
To: Vincent Choiniere
Cc: Tony Nguyen, Przemek Kitszel, intel-wired-lan,
Aleksandr Loktionov, Michal Kubiak, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] ice: fix Tx hang dump using stale tx_buf[0].skb
2026-10-16 13:08 ` Vincent Choiniere
(?)
(?)
@ 2026-10-17 13:08 ` sashiko-bot
-1 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-10-17 13:08 UTC (permalink / raw)
To: Vincent Choiniere, intel-wired-lan
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net/ice: TOCTOU race in ice_tx_hang_reporter_dump() causing SKB UAF
- [High] ice: out-of-bounds access in ice_tx_hang_reporter_dump() during ring resize
- [High] devlink: NULL pointer deref in ice_tx_hang_reporter_dump on teardown
--
commit 9b2375ee849517a745f8e8f71365535e40788bc1
Author: Vincent Choiniere <choiniere.vincent@hydroquebec.com>
ice: fix Tx hang dump using stale tx_buf[0].skb
This commit updates the ice devlink health reporter to read the SKB from the
Tx buffer at the next_to_clean index instead of slot 0. It also adds a type
check to ensure the buffer contains an SKB before dumping it.
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,
> 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];
[Severity: High]
Can event->tx_ring->tx_buf be NULL here?
If the interface is concurrently brought down, ice_free_tx_ring() frees the
tx_buf array and sets tx_ring->tx_buf to NULL.
Computing the address with the index offset and then evaluating tx_buf->type
below would trigger a page fault.
[Severity: High]
Could an out-of-bounds access occur if the ring is concurrently resized?
If a user runs ethtool -G to shrink the ring:
CPU1 (ethtool)
ice_set_ringparam()
ice_free_tx_ring(vsi->tx_rings[i]);
*vsi->tx_rings[i] = tx_rings[i];
CPU2 (Workqueue)
ice_tx_hang_reporter_dump()
tx_buf = &event->tx_ring->tx_buf[event->next_to_clean];
Since event->next_to_clean is cached from the timeout event, couldn't it
exceed the bounds of the newly allocated, smaller tx_buf array?
> + if (tx_buf->type == ICE_TX_BUF_SKB)
> + skb = tx_buf->skb;
[Severity: High]
Could this TOCTOU race happen with NAPI?
If this dump runs in a workqueue without disabling NAPI or taking a Tx queue
lock:
CPU1 (NAPI)
ice_clean_tx_irq()
napi_consume_skb(tx_buf->skb, napi_budget);
...
tx_buf->type = ICE_TX_BUF_EMPTY;
CPU2 (Workqueue)
ice_tx_hang_reporter_dump()
if (tx_buf->type == ICE_TX_BUF_SKB)
skb = tx_buf->skb;
...
if (skb)
devlink_fmsg_dump_skb(fmsg, skb);
If CPU1 frees the SKB after CPU2 performs the type check, doesn't
devlink_fmsg_dump_skb() end up dereferencing a freed pointer?
> 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));
[Severity: High]
Could event->tx_ring->desc also be NULL here if the ring was concurrently
freed?
Since ice_free_tx_ring() doesn't clear tx_ring->count, wouldn't
devlink_fmsg_binary_pair_put() attempt to copy from a NULL desc pointer?
> - devlink_fmsg_dump_skb(fmsg, skb);
> + if (skb)
> + devlink_fmsg_dump_skb(fmsg, skb);
> devlink_fmsg_obj_nest_end(fmsg);
>
> return 0;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-ice-tx-hang-dump-v1-1-3df0f5979a51@hydroquebec.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread