Re: [PATCH net] net: hns: retire service work before releasing device resources

[Jijie Shao <shaojijie@huawei.com>]

on 2026/10/16 10:00, Myeonghun Pak wrote:
> The remove path releases ring, PHY and accelerator resources before
> canceling service work, allowing use-after-free. A pending worker can
> also reinitialize the device or publish linkwatch events after
> unregister.
>
> Set REMOVING, synchronize an in-flight TX timeout, then shut down the
> service timer and drain work before unregister invokes ndo_stop.
> Serialize reset and link-update state checks and operations with
> unregister under RTNL.
>
> Compile-tested with Clang 22.1.2 (W=1); not tested on hardware.
>
> This issue was identified as part of our ongoing research while
> reviewing kernel code.
>
> 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>
> ---
>   drivers/net/ethernet/hisilicon/hns/hns_enet.c | 37 ++++++++++++++-----
>   1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> index e905f10b894e..977f054b00c3 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> @@ -2024,21 +2024,22 @@ static void hns_nic_reset_subtask(struct hns_nic_priv *priv)
>   		return;
>   	clear_bit(NIC_STATE2_RESET_REQUESTED, &priv->state);
>   
> -	/* If we're already down, removing or resetting, just bail */
> +	rtnl_lock();
> +	/* Serialize the reset decision with ndo_stop and unregister. */
>   	if (test_bit(NIC_STATE_DOWN, &priv->state) ||
>   	    test_bit(NIC_STATE_REMOVING, &priv->state) ||
>   	    test_bit(NIC_STATE_RESETTING, &priv->state))
> -		return;
> +		goto out;
>   
>   	hns_nic_dump(priv);

Since hns_nic_dump() can be highly time-consuming, is it really appropriate
to execute it inside the rtnl_lock critical section? Holding the global
RTNL lock for a long period might block the entire network control path
and impact system responsiveness.


>   	netdev_info(priv->netdev, "try to reset %s port!\n",
>   		    (type == HNAE_PORT_DEBUG ? "debug" : "service"));
>   
> -	rtnl_lock();
>   	/* put off any impending NetWatchDogTimeout */
>   	netif_trans_update(priv->netdev);
>   	hns_nic_net_reinit(priv->netdev);
>   
> +out:
>   	rtnl_unlock();
>   }
>   
> @@ -2058,7 +2059,14 @@ static void hns_nic_service_task(struct work_struct *work)
>   	struct hnae_handle *h = priv->ae_handle;
>   
>   	hns_nic_reset_subtask(priv);
> -	hns_nic_update_link_status(priv->netdev);
> +
> +	rtnl_lock();
> +	/* Keep linkwatch events within the netdev's registered lifetime. */
> +	if (!test_bit(NIC_STATE_DOWN, &priv->state) &&
> +	    !test_bit(NIC_STATE_REMOVING, &priv->state))
> +		hns_nic_update_link_status(priv->netdev);
> +	rtnl_unlock();
> +
>   	h->dev->ops->update_led_status(h);
>   	hns_nic_update_stats(priv->netdev);
>   
> @@ -2398,6 +2406,21 @@ 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);
> +	if (priv->notifier_block.notifier_call)
> +		hnae_unregister_notifier(&priv->notifier_block);
> +	priv->notifier_block.notifier_call = NULL;
> +
> +	/* Join a TX timeout that passed the REMOVING check before work drain. */
> +	if (ndev->reg_state != NETREG_UNINITIALIZED) {
> +		netif_tx_lock_bh(ndev);
> +		netif_tx_unlock_bh(ndev);
> +	}
> +
> +	/* Stop the timer and drain work before ndo_stop touches the hardware. */
> +	timer_shutdown_sync(&priv->service_timer);
> +	cancel_work_sync(&priv->service_task);

The commit message states "Serialize ... with unregister under RTNL",
but cancel_work_sync() has now been moved before unregister_netdev(),
which already drains the service_task.
Is the RTNL serialization in hns_nic_reset_subtask() still necessary?

> +
>   	if (ndev->reg_state != NETREG_UNINITIALIZED)
>   		unregister_netdev(ndev);
>   
> @@ -2411,12 +2434,6 @@ static void hns_nic_dev_remove(struct platform_device *pdev)
>   	if (!IS_ERR_OR_NULL(priv->ae_handle))
>   		hnae_put_handle(priv->ae_handle);
>   	priv->ae_handle = NULL;
> -	if (priv->notifier_block.notifier_call)
> -		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));