| Message ID | 20250114035118.110297-4-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: use netdev->lock to protect NAPI | expand |
On Tue, Jan 14, 2025 at 4:51 AM Jakub Kicinski <kuba@kernel.org> wrote: > > Protect writes to netdev->reg_state with netdev_lock(). > From now on holding netdev_lock() is sufficient to prevent > the net_device from getting unregistered, so code which > wants to hold just a single netdev around no longer needs > to hold rtnl_lock. > > We do not protect the NETREG_UNREGISTERED -> NETREG_RELEASED > transition. We'd need to move mutex_destroy(netdev->lock) > to .release, but the real reason is that trying to stop > the unregistration process mid-way would be unsafe / crazy. > Taking references on such devices is not safe, either. > So the intended semantics are to lock REGISTERED devices. Reviewed-by: Eric Dumazet <edumazet@google.com>
On Mon, Jan 13, 2025 at 07:51:09PM -0800, Jakub Kicinski wrote: > Protect writes to netdev->reg_state with netdev_lock(). > From now on holding netdev_lock() is sufficient to prevent > the net_device from getting unregistered, so code which > wants to hold just a single netdev around no longer needs > to hold rtnl_lock. > > We do not protect the NETREG_UNREGISTERED -> NETREG_RELEASED > transition. We'd need to move mutex_destroy(netdev->lock) > to .release, but the real reason is that trying to stop > the unregistration process mid-way would be unsafe / crazy. > Taking references on such devices is not safe, either. > So the intended semantics are to lock REGISTERED devices. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > include/linux/netdevice.h | 2 +- > net/core/dev.c | 6 ++++++ > 2 files changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Joe Damato <jdamato@fastly.com>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 0e008ce9d5ee..bdbc5849469c 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2448,7 +2448,7 @@ struct net_device { * Should always be taken using netdev_lock() / netdev_unlock() helpers. * Drivers are free to use it for other protection. * - * Protects: @net_shaper_hierarchy. + * Protects: @reg_state, @net_shaper_hierarchy. * Ordering: take after rtnl_lock. */ struct mutex lock; diff --git a/net/core/dev.c b/net/core/dev.c index 5c1e71afbe1c..2ded6eedb4cc 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10778,7 +10778,9 @@ int register_netdevice(struct net_device *dev) 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; @@ -11052,7 +11054,9 @@ void netdev_run_todo(void) continue; } + netdev_lock(dev); WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERED); + netdev_unlock(dev); linkwatch_sync_dev(dev); } @@ -11658,7 +11662,9 @@ void unregister_netdevice_many_notify(struct list_head *head, list_for_each_entry(dev, head, unreg_list) { /* And unlink it from device chain. */ unlist_netdevice(dev); + netdev_lock(dev); WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERING); + netdev_unlock(dev); } flush_all_backlogs();
Protect writes to netdev->reg_state with netdev_lock(). From now on holding netdev_lock() is sufficient to prevent the net_device from getting unregistered, so code which wants to hold just a single netdev around no longer needs to hold rtnl_lock. We do not protect the NETREG_UNREGISTERED -> NETREG_RELEASED transition. We'd need to move mutex_destroy(netdev->lock) to .release, but the real reason is that trying to stop the unregistration process mid-way would be unsafe / crazy. Taking references on such devices is not safe, either. So the intended semantics are to lock REGISTERED devices. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- include/linux/netdevice.h | 2 +- net/core/dev.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)