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

["Choinière, Vincent" <choiniere.vincent@hydroquebec.com>]

Thanks for the review. I posted v3 here:

https://lore.kernel.org/netdev/20261107-ice-tx-hang-dump-v3-1-8f22109d1aad@hydroquebec.com/

I checked the sequences against current net.

[High] VSI and ring lifetime in ice_remove():

You are right that the kernel-doc claim was too strong. In ice_remove(),
ice_unload() drops RTNL, ice_deinit() sets ICE_SERVICE_DIS and frees the
VSIs, and the service task is only cancelled later in ice_deinit_dev().
ice_service_task() also runs the Tx hang report before it looks at
ICE_SERVICE_DIS, so the rings are not only replaced or freed under RTNL
or from the service task.

The underlying lifetime issue predates this patch: before it, the service
task dereferences the same saved ring with no protection at all. v3
removes the incorrect claim about RTNL protection and documents the
remaining limitations in the kernel-doc (ethtool -G/-L and the reset paths
are covered; driver removal and devlink reload are not).

[High, pre-existing] reporter freed in ice_remove():

Correct. ice_health_deinit() destroys the reporter without clearing
pf->health_reporters.tx_hang, and the service task can still reach it.
Same cause as above: the service task is stopped after the teardown.

[High, pre-existing] DRIVER_REINIT reload vs. the service task:

I read the same sequence: ice_devlink_reinit_down() reaches
ice_deinit_dev() and cancel_work_sync() on the service task with the
devlink instance lock held, while devlink_health_report() takes that lock
when auto_dump is set. That is a lock-ordering problem rather than a
teardown-ordering one, so this patch does not address it. I have not
reproduced it, and dropping RTNL before reporting does not change it.

[Low] tx_hang_buf:

Yes, listed under Limitations and left out of this patch.

Przemek, Tony: I think these pre-existing teardown and lock-ordering
issues would be better addressed separately from this fix. I am willing
to investigate and test follow-up fixes (I can force Tx timeouts on an
E810 with KASAN and lockdep), but I am not an ice expert. Please let me
know whether you would like me to prepare a separate series, and I would
appreciate guidance on the preferred approach.