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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0 (1)

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] ice_tx_hang_snapshot()'s safety argument is that PF VSI rings
  'are only replaced or freed under RTNL or from the service task, so…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's
  new guard makes it look handled.
- [High] This problem wasn't introduced by this patch, but it is a
  deadlock between devlink DRIVER_REINIT reload and the Tx-hang…
- [Low] tx_hang_buf is published and consumed without synchronization.

--

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

This patch changes the ice Tx hang devlink dump so it no longer
dereferences tx_buf[0].skb or the saved tx_ring when the dump runs. It
records the queue and ring indices at timeout time. Under RTNL, it checks
that the saved ring is still one of the PF VSI Tx rings and that the VSI
is up, then copies the descriptors and IRQ name. It keeps only the skb
pointer value and drops RTNL before calling devlink_health_report().

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 8e9a8a8178d4..b9493ab4766e 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c

[ ... ]

> @@ -424,24 +429,104 @@ void ice_prep_tx_hang_report(struct ice_pf *pf, struct ice_tx_ring *tx_ring,
>  	buf->vsi_num = vsi_num;
>  	buf->head = head;
>  	buf->intr = intr;
> +	buf->queue = tx_ring->q_index;
> +	buf->next_to_clean = tx_ring->next_to_clean;
> +	buf->next_to_use = tx_ring->next_to_use;
> +}
> +
> +/**
> + * ice_tx_hang_snapshot - copy the hung Tx ring state for the dump
> + * @pf: PF on which the Tx hang was detected
> + * @tx_ring: ring saved by ice_tx_timeout(), possibly stale
> + * @ev: event to fill in
> + *
> + * The saved ring may have been freed or replaced since the timeout (e.g. by
> + * ethtool -G or -L). Use it only if it is still one of the PF VSI Tx rings
> + * and the VSI is up: those rings are only replaced or freed under RTNL or
> + * from the service task, so with RTNL held every access below is to live
> + * memory. ice_clean_tx_irq() may still run and free the skb at
> + * next_to_clean at any time, so only the pointer value is recorded and the
> + * skb itself is never dereferenced.
> + */
> +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);

[ ... ]

> +	ASSERT_RTNL();
> +
> +	if (!tx_ring || !vsi || !vsi->tx_rings ||
> +	    test_bit(ICE_VSI_DOWN, vsi->state))
> +		return;

[Severity: High]
Does the lifetime claim in the kernel-doc above hold for the ice_remove()
path?

In ice_remove(), RTNL is dropped inside ice_unload(). ice_deinit() then
tears down the VSIs without RTNL. The service work is only cancelled
later, in ice_deinit_dev():

    ice_remove()
        ice_unload(pf);        <- unregister_netdev() takes/drops RTNL
        devl_unlock(...);
        ice_deinit(pf);
            set_bit(ICE_SERVICE_DIS, pf->state);
            set_bit(ICE_DOWN, pf->state);
            ice_deinit_pf_sw(pf);  <- ice_vsi_release() frees rings,
                                      tx_rings and the vsi
            ice_dealloc_vsis(pf);  <- devm_kfree(..., pf->vsi)
        ...
        ice_deinit_dev(pf);    <- ice_service_task_stop()
                                  -> cancel_work_sync(&pf->serv_task)

ICE_SERVICE_DIS stops new scheduling, but it does not stop a work item
that is already queued or running. ice_vsi_free() holds only
pf->sw_mutex:

    pf->vsi[vsi->idx] = NULL;
    ...
    mutex_unlock(&pf->sw_mutex);
    devm_kfree(dev, vsi);

Suppose a Tx timeout fires shortly before removal. ice_service_task()
then sees tx_hang_buf.tx_ring set and calls ice_report_tx_hang() without
checking ICE_SERVICE_DIS. The work can block in rtnl_lock() while
unregister_netdev() holds RTNL. It then gets RTNL while ice_deinit() is
freeing the VSIs.

Could ice_get_main_vsi(pf), vsi->tx_rings and test_bit(ICE_VSI_DOWN,
vsi->state) then read the freed pf->vsi array or the freed struct ice_vsi?

The ICE_VSI_DOWN check only helps while the vsi itself is still
allocated. Does the service task need to be stopped or flushed, or
tx_hang_buf.tx_ring cleared, before the VSI teardown in ice_remove()?

[ ... ]

>  void ice_report_tx_hang(struct ice_pf *pf)
>  {
>  	struct ice_health_tx_hang_buf *buf = &pf->health_reporters.tx_hang_buf;
> -	struct ice_tx_ring *tx_ring = buf->tx_ring;
> -
>  	struct ice_tx_hang_event ev = {
>  		.head = buf->head,
>  		.intr = buf->intr,
>  		.vsi_num = buf->vsi_num,
> -		.queue = tx_ring->q_index,
> -		.next_to_clean = tx_ring->next_to_clean,
> -		.next_to_use = tx_ring->next_to_use,
> -		.tx_ring = tx_ring,
> +		.queue = buf->queue,
> +		.next_to_clean = buf->next_to_clean,
> +		.next_to_use = buf->next_to_use,
>  	};
>  
> +	if (!pf->health_reporters.tx_hang)
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but can this check pass with
a reporter that has already been freed?

In ice_remove(), the reporter is destroyed before ice_unload() and
ice_deinit(). The service work is still live at that point:

    ice_remove()
        ice_deinit_devlink(pf);
            ice_health_deinit(pf);
                ice_deinit_devl_reporter(pf->health_reporters.tx_hang);
                    devl_health_reporter_destroy()  <- kfree(reporter)
        ice_unload(pf);
        ...
        ice_deinit_dev(pf);    <- cancel_work_sync(&pf->serv_task)

ice_health_deinit() never sets pf->health_reporters.tx_hang to NULL, so
the stale pointer passes this check.

With a Tx timeout just before or during removal, the service task can
reach ice_devlink_health_report() after the kfree(). devlink_health_report()
then writes reporter->error_count and reporter->health_state, and it
dereferences reporter->devlink and reporter->ops.

Waiting in rtnl_lock() while unregister_netdev() holds RTNL also makes
it more likely that the report runs after the reporter has been freed.

> +
> +	rtnl_lock();
> +	ice_tx_hang_snapshot(pf, buf->tx_ring, &ev);
> +	rtnl_unlock();

[Severity: Low]
This is a pre-existing issue, and the changelog's limitations section
already covers it. The window looks wider now, though. ev.head, ev.intr,
ev.queue and the ring indices are read from tx_hang_buf before
rtnl_lock(). buf->tx_ring is only read after rtnl_lock() returns.

ice_prep_tx_hang_report() stores buf->tx_ring before the other fields,
with no ordering. ice_service_task() then clears the pointer with a
plain store:

    if (pf->health_reporters.tx_hang_buf.tx_ring) {
        ice_report_tx_hang(pf);
        pf->health_reporters.tx_hang_buf.tx_ring = NULL;
    }

Suppose a second timeout arrives while rtnl_lock() is blocked. Could the
dump then combine the first event's queue/head/intr with the second
event's ring snapshot? Could the clear above also drop the second event?

>  	ice_devlink_health_report(pf->health_reporters.tx_hang, "Tx hang", &ev);

[Severity: High]
This isn't a bug introduced by this patch, but can this deadlock against
a devlink driver_reinit reload?

ice_devlink_reinit_down() runs with the devlink instance lock held. It
ends up in cancel_work_sync() on the service work:

    ice_devlink_reload_down()
        ice_devlink_reinit_down()   <- devl_lock held by devlink core
            ice_unload(pf);
            ...
            ice_deinit_dev(pf);
                ice_service_task_stop()
                    cancel_work_sync(&pf->serv_task);

The tx_hang reporter has a dump op, so auto_dump is set. In that case
devlink_health_report() takes the same lock from the service work:

    if (reporter->auto_dump) {
        devl_lock(devlink);

Suppose a Tx timeout fires just before the reload. The service task can
then be inside ice_report_tx_hang(), possibly waiting for RTNL during
unregister_netdev(). It returns early from the snapshot because the VSI
is down and then blocks in devl_lock(). Meanwhile the reload thread waits
for that same work in cancel_work_sync().

Dropping RTNL before reporting avoids the RTNL/devlink lock ordering
problem. Does it do anything for this dependency between the reload and
the service work?

> +
> +	kvfree(ev.desc);
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-ice-tx-hang-dump-v2-1-e2136ef2c5dc%40hydroquebec.com