| Message ID | 20250114035118.110297-9-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: > > Now that NAPI instances can't come and go without holding > netdev->lock we can trivially switch from rtnl_lock() to > netdev_lock() for setting netdev->threaded via sysfs. > > Note that since we do not lock netdev_lock around sysfs > calls in the core we don't have to "trylock" like we do > with rtnl_lock. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: leitao@debian.org > --- > include/linux/netdevice.h | 13 +++++++++++-- > net/core/dev.c | 2 ++ > net/core/net-sysfs.c | 32 +++++++++++++++++++++++++++++++- > 3 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index d3108a12e562..75c30404657b 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -384,7 +384,7 @@ struct napi_struct { > int rx_count; /* length of rx_list */ > unsigned int napi_id; /* protected by netdev_lock */ > struct hrtimer timer; > - struct task_struct *thread; > + struct task_struct *thread; /* protected by netdev_lock */ > unsigned long gro_flush_timeout; > unsigned long irq_suspend_timeout; > u32 defer_hard_irqs; > @@ -2451,10 +2451,12 @@ struct net_device { > * Drivers are free to use it for other protection. > * > * Protects: > - * @napi_list, @net_shaper_hierarchy, @reg_state > + * @napi_list, @net_shaper_hierarchy, @reg_state, @threaded > * Partially protects (readers hold either @lock or rtnl_lock, > * writers must hold both for registered devices): > * @up > + * Also protects some fields in struct napi_struct. > + * > * Ordering: take after rtnl_lock. > */ > struct mutex lock; > @@ -2696,6 +2698,13 @@ static inline void netdev_assert_locked(struct net_device *dev) > lockdep_assert_held(&dev->lock); > } > > +static inline void netdev_assert_locked_or_invisible(struct net_device *dev) > +{ > + if (dev->reg_state == NETREG_REGISTERED || > + dev->reg_state == NETREG_UNREGISTERING) > + netdev_assert_locked(dev); > +} > + > static inline void netif_napi_set_irq(struct napi_struct *napi, int irq) > { > napi->irq = irq; > diff --git a/net/core/dev.c b/net/core/dev.c > index 1151baaedf4d..5872f0797cc3 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6784,6 +6784,8 @@ int dev_set_threaded(struct net_device *dev, bool threaded) > struct napi_struct *napi; > int err = 0; > > + netdev_assert_locked_or_invisible(dev); > + > if (dev->threaded == threaded) > return 0; > > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 2d9afc6e2161..5602a3c12e9a 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -108,6 +108,36 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr, > return ret; > } > > +/* Same as netdev_store() but takes netdev_lock() instead of rtnl_lock() */ > +static ssize_t > +netdev_lock_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len, > + int (*set)(struct net_device *, unsigned long)) > +{ > + struct net_device *netdev = to_net_dev(dev); > + struct net *net = dev_net(netdev); > + unsigned long new; > + int ret; > + > + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) > + return -EPERM; > + > + ret = kstrtoul(buf, 0, &new); > + if (ret) > + return ret; > + > + netdev_lock(netdev); > + > + if (dev_isalive(netdev)) { nit: dev_isalive() is supposed to be called with RCU or RTNL, I guess we should update its comment: /* Caller holds RTNL or RCU */ Reviewed-by: Eric Dumazet <edumazet@google.com>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index d3108a12e562..75c30404657b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -384,7 +384,7 @@ struct napi_struct { int rx_count; /* length of rx_list */ unsigned int napi_id; /* protected by netdev_lock */ struct hrtimer timer; - struct task_struct *thread; + struct task_struct *thread; /* protected by netdev_lock */ unsigned long gro_flush_timeout; unsigned long irq_suspend_timeout; u32 defer_hard_irqs; @@ -2451,10 +2451,12 @@ struct net_device { * Drivers are free to use it for other protection. * * Protects: - * @napi_list, @net_shaper_hierarchy, @reg_state + * @napi_list, @net_shaper_hierarchy, @reg_state, @threaded * Partially protects (readers hold either @lock or rtnl_lock, * writers must hold both for registered devices): * @up + * Also protects some fields in struct napi_struct. + * * Ordering: take after rtnl_lock. */ struct mutex lock; @@ -2696,6 +2698,13 @@ static inline void netdev_assert_locked(struct net_device *dev) lockdep_assert_held(&dev->lock); } +static inline void netdev_assert_locked_or_invisible(struct net_device *dev) +{ + if (dev->reg_state == NETREG_REGISTERED || + dev->reg_state == NETREG_UNREGISTERING) + netdev_assert_locked(dev); +} + static inline void netif_napi_set_irq(struct napi_struct *napi, int irq) { napi->irq = irq; diff --git a/net/core/dev.c b/net/core/dev.c index 1151baaedf4d..5872f0797cc3 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6784,6 +6784,8 @@ int dev_set_threaded(struct net_device *dev, bool threaded) struct napi_struct *napi; int err = 0; + netdev_assert_locked_or_invisible(dev); + if (dev->threaded == threaded) return 0; diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 2d9afc6e2161..5602a3c12e9a 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -108,6 +108,36 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr, return ret; } +/* Same as netdev_store() but takes netdev_lock() instead of rtnl_lock() */ +static ssize_t +netdev_lock_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t len, + int (*set)(struct net_device *, unsigned long)) +{ + struct net_device *netdev = to_net_dev(dev); + struct net *net = dev_net(netdev); + unsigned long new; + int ret; + + if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + return -EPERM; + + ret = kstrtoul(buf, 0, &new); + if (ret) + return ret; + + netdev_lock(netdev); + + if (dev_isalive(netdev)) { + ret = (*set)(netdev, new); + if (ret == 0) + ret = len; + } + netdev_unlock(netdev); + + return ret; +} + NETDEVICE_SHOW_RO(dev_id, fmt_hex); NETDEVICE_SHOW_RO(dev_port, fmt_dec); NETDEVICE_SHOW_RO(addr_assign_type, fmt_dec); @@ -638,7 +668,7 @@ static ssize_t threaded_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { - return netdev_store(dev, attr, buf, len, modify_napi_threaded); + return netdev_lock_store(dev, attr, buf, len, modify_napi_threaded); } static DEVICE_ATTR_RW(threaded);
Now that NAPI instances can't come and go without holding netdev->lock we can trivially switch from rtnl_lock() to netdev_lock() for setting netdev->threaded via sysfs. Note that since we do not lock netdev_lock around sysfs calls in the core we don't have to "trylock" like we do with rtnl_lock. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: leitao@debian.org --- include/linux/netdevice.h | 13 +++++++++++-- net/core/dev.c | 2 ++ net/core/net-sysfs.c | 32 +++++++++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 3 deletions(-)