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

[Myeonghun Pak]

Hi Jijie,

Thanks for the review. In v2, I will keep the dump outside RTNL and
remove the extra link-update locking. The original RTNL scope around
reinit will remain to serialize with ndo_stop(), with a state recheck
after acquiring the lock. I will clarify that draining work before
unregister makes additional serialization against unregister
unnecessary.

Best regards,
Myeonghun

2026년 10월 19일 (월) 오전 3:00, Jijie Shao <shaojijie@xxxxxxxxxx>님이 작성:
>
>
> 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@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> >   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));