[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.

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: hns: stop service work before device resource teardown

This patch moves the NIC_STATE_REMOVING setting and the service_task
cancellation in hns_nic_dev_remove() ahead of unregister_netdev() and the
ring/AE teardown. It also joins any in-flight ndo_tx_timeout() and shuts
down the service timer first. hns_nic_reset_subtask() now rechecks the
DOWN, REMOVING and RESETTING flags after taking RTNL.

> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> index e905f10b894e..595fe3514056 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> @@ -2035,10 +2035,17 @@ static void hns_nic_reset_subtask(struct hns_nic_priv *priv)
>  		    (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;

[Severity: Low]
This is a pre-existing issue, but can the NIC_STATE_RESETTING test added
here ever be true?

NIC_STATE_RESETTING and NIC_STATE_REINITING are only set by the
test_and_set_bit()/usleep_range() loops in hns_nic_net_reset() and
hns_nic_net_reinit():

hns_nic_net_reset() {
	while (test_and_set_bit(NIC_STATE_RESETTING, &priv->state))
		usleep_range(1000, 2000);
	...
	clear_bit(NIC_STATE_RESETTING, &priv->state);
}

Every caller appears to hold RTNL already. hns_nic_net_reset() is reached
from the ethtool self-test path (__lb_up(), __lb_down(),
hns_nic_self_test()) and from hns_nic_net_reinit(). The only caller of
hns_nic_net_reinit() is hns_nic_reset_subtask(), under rtnl_lock().

That would make this recheck dead code once rtnl_lock() is held. The bit
loops also use flags as locks, which lockdep cannot see. Would
lockdep_assert_rtnl_held() or a real mutex be clearer there?

> +
>  	/* put off any impending NetWatchDogTimeout */
>  	netif_trans_update(priv->netdev);
>  	hns_nic_net_reinit(priv->netdev);
>  
> +out:
>  	rtnl_unlock();
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but the rest of
hns_nic_service_task() still runs without RTNL after this rtnl_unlock():

hns_nic_service_task() {
	...
	hns_nic_reset_subtask(priv);
	hns_nic_update_link_status(priv->netdev);
	h->dev->ops->update_led_status(h);
	hns_nic_update_stats(priv->netdev);
	...
}

hns_nic_net_down() (ndo_stop) only stops the timer. It does not wait for
a service_task that is already queued or running:

hns_nic_net_down() {
	...
	(void) timer_delete_sync(&priv->service_timer);
	netif_tx_stop_all_queues(ndev);
	netif_carrier_off(ndev);
	netif_tx_disable(ndev);
	priv->link = 0;
	...
}

Could a worker in hns_nic_update_link_status()->hns_nic_adjust_link()
then call netif_carrier_on() and netif_tx_wake_all_queues() on the stopped
device? It could also set priv->link back to 1, or call
need_adjust_link()/adjust_link() on the hardware while ops->stop() is
running.

With an XGMII PHY, the worker also calls genphy_read_status() and
hns_nic_adjust_link() without phydev->lock. That can run at the same time
as the phylib state machine runs the same adjust_link callback.

hns_ae_update_stats() appears to read only registers and ring->stats, and
ndo_stop does not free those. So the result looks like inconsistent
carrier/link state, not memory corruption. The v2 notes say RTNL around
the link-status update was dropped on purpose.

>  
> @@ -2398,6 +2405,18 @@ static void hns_nic_dev_remove(struct platform_device *pdev)
>  	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);

[Severity: High]
This is a pre-existing issue, but what serialises hns_nic_dev_remove()
against the HNAE AE-registration notifier?

If hns_nic_try_get_ae() failed during probe, the NIC stays on the raw
ae_chain. Remove then reads ndev->reg_state without a lock, both in the
new tx lock join above and here. It tears down ring_data, the PHY and
ae_handle, and only then calls hnae_unregister_notifier().

Meanwhile, a DSAF AE probe on another CPU can run:

hnae_ae_register()
  raw_notifier_call_chain(&ae_chain, HNAE_AE_REGISTER, NULL)
    hns_nic_notifier_action()
      hns_nic_try_get_ae()
        hnae_get_handle()
        hns_nic_init_phy()
        hns_nic_init_ring_data()
        register_netdev(ndev)

ae_chain is a RAW_NOTIFIER_HEAD. hnae_register_notifier() and
hnae_unregister_notifier() take no lock, and hns_nic_notifier_action()
does not check NIC_STATE_REMOVING.

If remove saw NETREG_UNINITIALIZED (or ring_data == NULL) before the
callback finished, can free_netdev() run on a netdev that is now
registered? In that case free_netdev() frees the queues and then hits:

	BUG_ON(dev->reg_state != NETREG_UNREGISTERED);

The core would also keep a reference to the freed netdev, and the new
handle and rings would leak. Remove can also set priv->ae_handle to NULL
while the callback is still using it.

raw_notifier_chain_unregister() only unlinks the entry and does not wait
for a running callback. Moving the unregister earlier would not be enough
on its own.

This does not affect the service work fix in this patch: REMOVING is set
first and the timer is shut down. The commit message also says the
notifier ordering was left alone on purpose.

[ ... ]
-- 
pw-bot: cr