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/[email protected]/ > 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
