[PATCH net v2] net: hns: stop service work before device resource teardown

[PATCH net v2] net: hns: stop service work before device resource teardown

[Myeonghun Pak]

hns_nic_dev_remove() unregisters the netdev and releases ring and AE
resources before cancelling service_task. An already running service
worker can therefore use the released ring or AE handle.

Set NIC_STATE_REMOVING before teardown, join any in-flight TX watchdog
callback which could have passed the scheduling check, and shut down the
service timer before draining the work. Drain service_task before
unregister_netdev() and resource teardown. Call cancel_work_sync() without
holding RTNL, since the service worker may need to acquire it before
completing.

The service worker already takes RTNL for hns_nic_net_reinit(). Recheck the
DOWN, REMOVING and RESETTING flags after taking RTNL: ndo_stop() may have
run while the diagnostic dump was executing. Leave hns_nic_dump() outside
the RTNL critical section because dumping all device registers can take
a substantial amount of time.

Fixes: b5996f11ea54 ("net: add Hisilicon Network Subsystem basic ethernet support")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
v2:
- Keep hns_nic_dump() outside RTNL and recheck state under RTNL.
- Drop extra RTNL locking around the link-status update.
- Leave notifier cleanup in its original location since the HNAE notifier
  does not directly queue the service work.

 drivers/net/ethernet/hisilicon/hns/hns_enet.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -2035,10 +2035,17 @@
 		    (type == HNAE_PORT_DEBUG ? "debug" : "service"));
 
 	rtnl_lock();
+	/* ndo_stop may have run while the diagnostic dump was in progress. */
+	if (test_bit(NIC_STATE_DOWN, &priv->state) ||
+	    test_bit(NIC_STATE_REMOVING, &priv->state) ||
+	    test_bit(NIC_STATE_RESETTING, &priv->state))
+		goto out;
+
 	/* put off any impending NetWatchDogTimeout */
 	netif_trans_update(priv->netdev);
 	hns_nic_net_reinit(priv->netdev);
 
+out:
 	rtnl_unlock();
 }
 
@@ -2397,6 +2404,18 @@
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct hns_nic_priv *priv = netdev_priv(ndev);
+
+	set_bit(NIC_STATE_REMOVING, &priv->state);
+
+	/* Join any ndo_tx_timeout() which might have queued service_task. */
+	if (ndev->reg_state != NETREG_UNINITIALIZED) {
+		netif_tx_lock_bh(ndev);
+		netif_tx_unlock_bh(ndev);
+	}
+
+	/* Stop the timer producer before draining the service worker. */
+	timer_shutdown_sync(&priv->service_timer);
+	cancel_work_sync(&priv->service_task);
 
 	if (ndev->reg_state != NETREG_UNINITIALIZED)
 		unregister_netdev(ndev);
@@ -2415,9 +2434,6 @@
 		hnae_unregister_notifier(&priv->notifier_block);
 	priv->notifier_block.notifier_call = NULL;
 
-	set_bit(NIC_STATE_REMOVING, &priv->state);
-	(void)cancel_work_sync(&priv->service_task);
-
 	/* safe for ACPI FW */
 	of_node_put(to_of_node(priv->fwnode));
 
-- 
2.52.0

Re: [PATCH net v2] net: hns: stop service work before device resource teardown

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.