* [PATCH net] net: hns: retire service work before releasing device resources
@ 2026-10-16 2:00 Myeonghun Pak
2026-10-19 7:00 ` Jijie Shao
2026-10-20 2:15 ` netdev-bot+sashiko
0 siblings, 2 replies; 4+ messages in thread
From: Myeonghun Pak @ 2026-10-16 2:00 UTC (permalink / raw)
To: Jian Shen, Jijie Shao, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, Myeonghun Pak, Ijae Kim
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);
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);
+
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));
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: hns: retire service work before releasing device resources
2026-10-16 2:00 [PATCH net] net: hns: retire service work before releasing device resources Myeonghun Pak
@ 2026-10-19 7:00 ` Jijie Shao
2026-10-19 23:23 ` Myeonghun Pak
2026-10-20 2:15 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Jijie Shao @ 2026-10-19 7:00 UTC (permalink / raw)
To: Myeonghun Pak, Jian Shen, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: shaojijie, netdev, linux-kernel, Ijae Kim
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));
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: hns: retire service work before releasing device resources
2026-10-19 7:00 ` Jijie Shao
@ 2026-10-19 23:23 ` Myeonghun Pak
0 siblings, 0 replies; 4+ messages in thread
From: Myeonghun Pak @ 2026-10-19 23:23 UTC (permalink / raw)
To: Jijie Shao
Cc: Jian Shen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Ijae Kim
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@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));
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: hns: retire service work before releasing device resources
2026-10-16 2:00 [PATCH net] net: hns: retire service work before releasing device resources Myeonghun Pak
2026-10-19 7:00 ` Jijie Shao
@ 2026-10-20 2:15 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-20 2:15 UTC (permalink / raw)
To: mhun512
Cc: shenjian15, shaojijie, andrew+netdev, davem, edumazet, kuba,
pabeni, netdev, linux-kernel, ae878000
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] hns_nic_service_task() now takes rtnl_lock() on every run of
the 1 Hz service work, on every port, around…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
reachable kernel BUG.
- [High] This problem wasn't introduced by this patch, but the remove
path still races with an AE-register notifier callback that is already…
--
net: hns: retire service work before releasing device resources
This patch reorders hns_nic_dev_remove(). It now sets NIC_STATE_REMOVING,
unregisters the AE notifier, waits for any TX timeout already running, and
drains the service timer and work before unregister_netdev(). It also takes
RTNL earlier in hns_nic_reset_subtask() and wraps the link status update in
hns_nic_service_task() with RTNL.
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> index e905f10b894e8..977f054b00c31 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
[ ... ]
> @@ -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();
[Severity: Medium]
Can this hold the global RTNL mutex for 200 ms or more on every service
cycle?
hns_nic_service_timer() re-arms every SERVICE_TIMER_HZ and schedules this
work. That means every port now takes RTNL once a second. Before this
patch, RTNL was only taken when NIC_STATE2_RESET_REQUESTED was set.
On a port where h->phy_dev is set and the PHY mode is
PHY_INTERFACE_MODE_XGMII, the locked section runs genphy_read_status()
and then hns_nic_adjust_link():
hns_nic_adjust_link() {
...
if (h->dev->ops->need_adjust_link(h, ndev->phydev->speed,
ndev->phydev->duplex)) {
...
netif_carrier_off(ndev);
msleep(200);
h->dev->ops->adjust_link(h, ndev->phydev->speed,
ndev->phydev->duplex);
...
}
On XGMAC ports, hns_xgmac_config() never sets mac_drv->need_adjust_link,
so hns_mac_need_adjust_link() always returns true:
hns_mac_need_adjust_link() {
...
if (mac_ctrl_drv->need_adjust_link)
return mac_ctrl_drv->need_adjust_link(mac_ctrl_drv,
(enum mac_speed)speed, duplex);
else
return true;
}
So with the link up, msleep(200) runs under RTNL on every cycle. On AE v2,
hns_ae_adjust_link() also calls hns_ae_wait_flow_down(). This blocks
rtnetlink and every other RTNL user system-wide, and the delay adds up
across ports.
Is RTNL needed here for the reason given in the comment?
hns_nic_dev_remove() now sets NIC_STATE_REMOVING and calls
timer_shutdown_sync() and cancel_work_sync() before unregister_netdev().
Any running worker therefore finishes while the netdev is still
registered. The lock seems to add only serialization of the
NIC_STATE_DOWN check against ndo_stop.
[ ... ]
> @@ -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;
[Severity: High]
This is a pre-existing issue, but can hns_nic_dev_remove() still race with
an hns_nic_notifier_action() callback that is already running?
ae_chain is a RAW_NOTIFIER_HEAD. DSAF probe walks it with no lock:
hnae_ae_register() {
...
ret = raw_notifier_call_chain(&ae_chain, HNAE_AE_REGISTER, NULL);
...
}
hnae_unregister_notifier() only calls raw_notifier_chain_unregister().
That unlinks the entry but does not wait for a callback that is already
running. DSAF probe and enet remove run under different device locks, so
nothing orders them.
Suppose the callback is inside hns_nic_try_get_ae() when remove starts:
CPU1 (DSAF probe)
hnae_ae_register()
raw_notifier_call_chain()
hns_nic_notifier_action()
hns_nic_try_get_ae()
hnae_get_handle()
hns_nic_init_ring_data()
register_netdev()
CPU2 (enet unbind)
hns_nic_dev_remove()
set_bit(NIC_STATE_REMOVING, ...)
hnae_unregister_notifier()
timer_shutdown_sync() / cancel_work_sync()
reg_state still NETREG_UNINITIALIZED -> skips unregister_netdev()
ring_data still NULL -> skips hns_nic_uninit_ring_data()
ae_handle still NULL -> skips hnae_put_handle()
free_netdev()
Could free_netdev() then free a netdev that is partway through
registration? That would leak the handle, the module and cls_dev
references, and ring_data. The callback and remove could also both
release the handle at the same time.
After hns_nic_try_get_ae() returns, the callback also writes to priv:
if (!hns_nic_try_get_ae(priv->netdev)) {
hnae_unregister_notifier(&priv->notifier_block);
priv->notifier_block.notifier_call = NULL;
}
That write would land in freed memory, and the chain walk may then read
nb->next from the freed priv.
hns_nic_notifier_action() never checks NIC_STATE_REMOVING. Moving the
notifier unregistration to the top of remove makes the window smaller but
does not seem to close it.
> +
> + /* 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);
> +
> if (ndev->reg_state != NETREG_UNINITIALIZED)
> unregister_netdev(ndev);
[Severity: High]
This isn't a bug introduced by this patch, but is
reg_state != NETREG_UNINITIALIZED the right test for unregister_netdev()?
The patch uses the same test for the new netif_tx_lock_bh() barrier. The
barrier itself is harmless, because alloc_netdev allocated the TX queues.
If register_netdevice() fails in netdev_register_kobject(), reg_state is
left as NETREG_UNREGISTERED:
net/core/dev.c:register_netdevice() {
...
ret = netdev_register_kobject(dev);
netdev_lock(dev);
WRITE_ONCE(dev->reg_state, ret ? NETREG_UNREGISTERED : NETREG_REGISTERED);
netdev_unlock(dev);
if (ret)
goto err_uninit_notify;
...
}
A NETDEV_REGISTER notifier veto also ends in NETREG_UNREGISTERED once
netdev_run_todo() runs.
hns_nic_try_get_ae() handles this as an ordinary failure through
out_reg_ndev_fail. hns_nic_dev_probe() then registers the AE notifier and
returns 0, so the driver stays bound. On a later unbind or rmmod:
hns_nic_dev_remove()
unregister_netdev()
...
unregister_netdevice_many_notify()
BUG_ON(dev->reg_state != NETREG_REGISTERED);
Should both guards test reg_state == NETREG_REGISTERED instead?
There is a related case. If the AE notifier, which is still registered,
fires later, hns_nic_try_get_ae() calls register_netdev() again on the
same device. register_netdevice() would then hit:
BUG_ON(dev->reg_state != NETREG_UNINITIALIZED);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016020035.3621768-1-mhun512%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-20 2:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-16 2:00 [PATCH net] net: hns: retire service work before releasing device resources Myeonghun Pak
2026-10-19 7:00 ` Jijie Shao
2026-10-19 23:23 ` Myeonghun Pak
2026-10-20 2:15 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®