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

[Przemek Kitszel]

On 10/26/26 2:30 PM, Vincent Choiniere via B4 Relay wrote:
> From: Vincent Choiniere <choiniere.vincent@xxxxxxxxxxxxxxx>
>
> 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.
>
> Using tx_buf[next_to_clean] alone does not fix the lifetime issues.
> The dump runs from the service task without synchronization with Tx
> cleanup or ring replacement:
>
> - ice_clean_tx_irq() can free the skb at next_to_clean while the
>    report is being prepared. It calls napi_consume_skb() before
>    marking the buffer empty.
> - The ring saved by ice_tx_timeout() can be replaced or freed before
>    the service task runs. ethtool -G replaces the ring contents and
>    frees tx_buf and desc, while ethtool -L can free whole rings.
>
> Record the queue index, next_to_clean and next_to_use at timeout time.
> Before reporting, take RTNL and check that the saved ring is still
> one of the PF VSI Tx rings and that the VSI is up. Copy the
> descriptors and IRQ name while the ring storage is protected against
> replacement. If the ring cannot be used or the copy cannot be made,
> report only the values recorded at timeout time.
>
> Tx cleanup can still run while the ring state is copied, so do not
> dereference the skb. Retain only its pointer value in the dump and
> remove the skb contents.
>
> Release RTNL before calling devlink_health_report(), which takes the
> devlink instance lock. Build the dump from the copied data and free
> the descriptor copy after reporting.
>
> Observed on an E810 PF passed through to a KVM guest with Ubuntu
> kernel 7.0.0-14 (PREEMPT_RT and a 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]
>
> Fixes: 2a82874a3b7b ("ice: add Tx hang devlink health reporter")
> Link: https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@xxxxxxxxx/
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Vincent Choiniere <choiniere.vincent@xxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Validate the saved ring under RTNL before dereferencing it from
>    the service task, and restrict snapshots to the PF VSI while it
>    is up (Sashiko).
> - Check next_to_clean against count before indexing tx_buf, and
>    copy the descriptors and IRQ name while ring replacement is
>    excluded (Sashiko).
> - Stop dereferencing the skb and remove its contents from the dump.
>    Keep only its pointer value (Sashiko).
> - Release RTNL before reporting to preserve the devlink instance
>    lock ordering.
> - Record the queue index and ring indices at timeout time as a
>    fallback when a snapshot cannot be made.
> - Drop Paul's Reviewed-by because the patch changed substantially.

lot's of changes and code, but rather nothing controversial, I'm
generally happy with it, one nitpick from me inline, and otherwise:

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>

> +static void ice_tx_hang_snapshot(struct ice_pf *pf, struct ice_tx_ring *tx_ring,
> +				 struct ice_tx_hang_event *ev)
> +{
> +	struct ice_vsi *vsi = ice_get_main_vsi(pf);
> +	struct ice_q_vector *q_vector;
> +	struct ice_tx_buf *tx_buf;
> +	unsigned int i;
> +	u16 ntc, ntu;
> +	void *desc;
> +	u32 size;
> +
> +	ASSERT_RTNL();
> +
> +	if (!tx_ring || !vsi || !vsi->tx_rings ||
> +	    test_bit(ICE_VSI_DOWN, vsi->state))
> +		return;
> +
> +	ice_for_each_txq(vsi, i)
> +		if (vsi->tx_rings[i] == tx_ring)
> +			break;
> +	if (i == vsi->num_txq)
> +		return;
> +
> +	q_vector = tx_ring->q_vector;
> +	if (!q_vector || !tx_ring->desc || !tx_ring->tx_buf || !tx_ring->count)
> +		return;
> +
> +	size = tx_ring->count * sizeof(struct ice_tx_desc);
> +	desc = kvmalloc(size, GFP_KERNEL);
> +	if (!desc)
> +		return;
> +
> +	ntc = READ_ONCE(tx_ring->next_to_clean);
> +	ntu = READ_ONCE(tx_ring->next_to_use);
> +
> +	if (ntc < tx_ring->count) {
> +		tx_buf = &tx_ring->tx_buf[ntc];
> +		if (tx_buf->type == ICE_TX_BUF_SKB)
> +			ev->skb_ptr = READ_ONCE(tx_buf->skb);
> +
> +		memcpy(desc, tx_ring->desc, size);
> +		ev->next_to_clean = ntc;
> +		ev->next_to_use = ntu;
> +		strscpy(ev->irq_name, q_vector->name);
> +		ev->desc_ptr = tx_ring->desc;
> +		ev->dma = tx_ring->dma;
> +		ev->desc_size = size;
> +		ev->desc = desc;
> +	}
> +
> +	if (!ev->desc)

this is abuse of ev and bad style, instead just glue an else to the
previous if, basically:
     if (ntc < tx_ring->count) { .... } else { kvfree(desc); }

> +		kvfree(desc);
>   }