| Message ID | 20250115035319.559603-3-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: use netdev->lock to protect NAPI | expand |
From: Jakub Kicinski <kuba@kernel.org> Date: Tue, 14 Jan 2025 19:53:10 -0800 > 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: Joe Damato <jdamato@fastly.com> > Reviewed-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > v2: > - reorder with next patch > v1: https://lore.kernel.org/20250114035118.110297-4-kuba@kernel.org > --- > include/linux/netdevice.h | 2 +- > net/core/dev.c | 6 ++++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 891c5bdb894c..30963c5d409b 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 fda4e1039bf0..6603c08768f6 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -10668,7 +10668,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); Do we need the lock before list_netdevice() ? It's not a big deal, so Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 891c5bdb894c..30963c5d409b 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 fda4e1039bf0..6603c08768f6 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10668,7 +10668,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; @@ -10942,7 +10944,9 @@ void netdev_run_todo(void) continue; } + netdev_lock(dev); WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERED); + netdev_unlock(dev); linkwatch_sync_dev(dev); } @@ -11548,7 +11552,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();