| Message ID | 20250327135659.2057487-4-sdf@fomichev.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hold instance lock during NETDEV_UP/REGISTER/UNREGISTER | expand |
On Thu, 27 Mar 2025 06:56:51 -0700 Stanislav Fomichev wrote: > @@ -3151,11 +3153,12 @@ int addrconf_add_ifaddr(struct net *net, void __user *arg) > cfg.plen = ireq.ifr6_prefixlen; > > rtnl_net_lock(net); > - dev = __dev_get_by_index(net, ireq.ifr6_ifindex); > + dev = netdev_get_by_index_lock(net, ireq.ifr6_ifindex); I think you want ops locking here, no? netdev_get_by_index_lock() will also lock devs which didn't opt in.
On 03/27, Jakub Kicinski wrote: > On Thu, 27 Mar 2025 06:56:51 -0700 Stanislav Fomichev wrote: > > @@ -3151,11 +3153,12 @@ int addrconf_add_ifaddr(struct net *net, void __user *arg) > > cfg.plen = ireq.ifr6_prefixlen; > > > > rtnl_net_lock(net); > > - dev = __dev_get_by_index(net, ireq.ifr6_ifindex); > > + dev = netdev_get_by_index_lock(net, ireq.ifr6_ifindex); > > I think you want ops locking here, no? > netdev_get_by_index_lock() will also lock devs which didn't opt in. New netdev_get_by_index_lock_ops? I felt like we already have too many xxxdev_get_by, but agreed that it should be safer, will do!
On Thu, 27 Mar 2025 14:06:43 -0700 Stanislav Fomichev wrote: > On 03/27, Jakub Kicinski wrote: > > On Thu, 27 Mar 2025 06:56:51 -0700 Stanislav Fomichev wrote: > > > @@ -3151,11 +3153,12 @@ int addrconf_add_ifaddr(struct net *net, void __user *arg) > > > cfg.plen = ireq.ifr6_prefixlen; > > > > > > rtnl_net_lock(net); > > > - dev = __dev_get_by_index(net, ireq.ifr6_ifindex); > > > + dev = netdev_get_by_index_lock(net, ireq.ifr6_ifindex); > > > > I think you want ops locking here, no? > > netdev_get_by_index_lock() will also lock devs which didn't opt in. > > New netdev_get_by_index_lock_ops? I felt like we already have too many > xxxdev_get_by, but agreed that it should be safer, will do! I think we're holding rtnl_lock here, so we don't need an "atomic get and lock", we can stick to __dev_get_by_index() and then lock it separately?
On 03/27, Jakub Kicinski wrote: > On Thu, 27 Mar 2025 14:06:43 -0700 Stanislav Fomichev wrote: > > On 03/27, Jakub Kicinski wrote: > > > On Thu, 27 Mar 2025 06:56:51 -0700 Stanislav Fomichev wrote: > > > > @@ -3151,11 +3153,12 @@ int addrconf_add_ifaddr(struct net *net, void __user *arg) > > > > cfg.plen = ireq.ifr6_prefixlen; > > > > > > > > rtnl_net_lock(net); > > > > - dev = __dev_get_by_index(net, ireq.ifr6_ifindex); > > > > + dev = netdev_get_by_index_lock(net, ireq.ifr6_ifindex); > > > > > > I think you want ops locking here, no? > > > netdev_get_by_index_lock() will also lock devs which didn't opt in. > > > > New netdev_get_by_index_lock_ops? I felt like we already have too many > > xxxdev_get_by, but agreed that it should be safer, will do! > > I think we're holding rtnl_lock here, so we don't need an > "atomic get and lock", we can stick to __dev_get_by_index() > and then lock it separately? Sounds good, that should do it, thanks!
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index fa79145518d1..b2b4e31806d5 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3386,6 +3386,7 @@ struct net_device *dev_get_by_index(struct net *net, int ifindex); struct net_device *__dev_get_by_index(struct net *net, int ifindex); struct net_device *netdev_get_by_index(struct net *net, int ifindex, netdevice_tracker *tracker, gfp_t gfp); +struct net_device *netdev_get_by_index_lock(struct net *net, int ifindex); struct net_device *netdev_get_by_name(struct net *net, const char *name, netdevice_tracker *tracker, gfp_t gfp); struct net_device *dev_get_by_index_rcu(struct net *net, int ifindex); diff --git a/net/core/dev.c b/net/core/dev.c index 019f838f94d8..bb4a135b1569 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1072,6 +1072,7 @@ struct net_device *netdev_get_by_index_lock(struct net *net, int ifindex) return __netdev_put_lock(dev); } +EXPORT_SYMBOL(netdev_get_by_index_lock); struct net_device * netdev_xa_find_lock(struct net *net, struct net_device *dev, diff --git a/net/core/dev.h b/net/core/dev.h index 7ee203395d8e..8d35860f2e89 100644 --- a/net/core/dev.h +++ b/net/core/dev.h @@ -28,7 +28,6 @@ struct napi_struct * netdev_napi_by_id_lock(struct net *net, unsigned int napi_id); struct net_device *dev_get_by_napi_id(unsigned int napi_id); -struct net_device *netdev_get_by_index_lock(struct net *net, int ifindex); struct net_device *__netdev_put_lock(struct net_device *dev); struct net_device * netdev_xa_find_lock(struct net *net, struct net_device *dev, diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index ac8cc1076536..665184d2adce 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -80,6 +80,7 @@ #include <net/netlink.h> #include <net/pkt_sched.h> #include <net/l3mdev.h> +#include <net/netdev_lock.h> #include <linux/if_tunnel.h> #include <linux/rtnetlink.h> #include <linux/netconf.h> @@ -377,6 +378,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev) int err = -ENOMEM; ASSERT_RTNL(); + netdev_ops_assert_locked(dev); if (dev->mtu < IPV6_MIN_MTU && dev != blackhole_netdev) return ERR_PTR(-EINVAL); @@ -402,7 +404,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev) return ERR_PTR(err); } if (ndev->cnf.forwarding) - dev_disable_lro(dev); + netif_disable_lro(dev); /* We refer to the device */ netdev_hold(dev, &ndev->dev_tracker, GFP_KERNEL); @@ -3151,11 +3153,12 @@ int addrconf_add_ifaddr(struct net *net, void __user *arg) cfg.plen = ireq.ifr6_prefixlen; rtnl_net_lock(net); - dev = __dev_get_by_index(net, ireq.ifr6_ifindex); + dev = netdev_get_by_index_lock(net, ireq.ifr6_ifindex); if (dev) err = inet6_addr_add(net, dev, &cfg, 0, 0, NULL); else err = -ENODEV; + netdev_unlock(dev); rtnl_net_unlock(net); return err; } @@ -5022,11 +5025,11 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh, rtnl_net_lock(net); - dev = __dev_get_by_index(net, ifm->ifa_index); + dev = netdev_get_by_index_lock(net, ifm->ifa_index); if (!dev) { NL_SET_ERR_MSG_MOD(extack, "Unable to find the interface"); err = -ENODEV; - goto unlock; + goto unlock_rtnl; } idev = ipv6_find_idev(dev); @@ -5065,6 +5068,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh, in6_ifa_put(ifa); unlock: + netdev_unlock(dev); +unlock_rtnl: rtnl_net_unlock(net); return err; @@ -6503,7 +6508,9 @@ static int addrconf_sysctl_addr_gen_mode(const struct ctl_table *ctl, int write, if (idev->cnf.addr_gen_mode != new_val) { WRITE_ONCE(idev->cnf.addr_gen_mode, new_val); + netdev_lock_ops(idev->dev); addrconf_init_auto_addrs(idev->dev); + netdev_unlock_ops(idev->dev); } } else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) { struct net_device *dev; @@ -6515,7 +6522,9 @@ static int addrconf_sysctl_addr_gen_mode(const struct ctl_table *ctl, int write, idev->cnf.addr_gen_mode != new_val) { WRITE_ONCE(idev->cnf.addr_gen_mode, new_val); + netdev_lock_ops(idev->dev); addrconf_init_auto_addrs(idev->dev); + netdev_unlock_ops(idev->dev); } } }
ipv6_add_dev might call dev_disable_lro which unconditionally grabs instance lock, so it will deadlock during NETDEV_REGISTER. Switch to netif_disable_lro. Make sure all callers hold the instance lock as well. Cc: Cosmin Ratiu <cratiu@nvidia.com> Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations") Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- include/linux/netdevice.h | 1 + net/core/dev.c | 1 + net/core/dev.h | 1 - net/ipv6/addrconf.c | 17 +++++++++++++---- 4 files changed, 15 insertions(+), 5 deletions(-)