Message ID | 20250311084507.3978048-3-sdf@fomichev.me (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: bring back dev_addr_sem | expand |
From: Stanislav Fomichev <sdf@fomichev.me> Date: Tue, 11 Mar 2025 01:45:07 -0700 > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 9355058bf996..c9d44dad203d 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3080,21 +3080,32 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, > struct sockaddr *sa; > int len; > > + netdev_unlock_ops(dev); > + > + /* dev_addr_sem is an outer lock, enforce proper ordering */ > + down_write(&dev_addr_sem); > + netdev_lock_ops(dev); > + > len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, > sizeof(*sa)); > sa = kmalloc(len, GFP_KERNEL); > if (!sa) { > + up_write(&dev_addr_sem); > err = -ENOMEM; > goto errout; > } > sa->sa_family = dev->type; > memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), > dev->addr_len); Can we move down_write() and netdev_lock_ops() here ? > - err = netif_set_mac_address_user(dev, sa, extack); > + err = netif_set_mac_address(dev, sa, extack); > kfree(sa); > - if (err) > + if (err) { > + up_write(&dev_addr_sem); > goto errout; > + } > status |= DO_SETLINK_MODIFIED; > + > + up_write(&dev_addr_sem); > } > > if (tb[IFLA_MTU]) { > -- > 2.48.1
On 03/11, Kuniyuki Iwashima wrote: > From: Stanislav Fomichev <sdf@fomichev.me> > Date: Tue, 11 Mar 2025 01:45:07 -0700 > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > > index 9355058bf996..c9d44dad203d 100644 > > --- a/net/core/rtnetlink.c > > +++ b/net/core/rtnetlink.c > > @@ -3080,21 +3080,32 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, > > struct sockaddr *sa; > > int len; > > > > + netdev_unlock_ops(dev); > > + > > + /* dev_addr_sem is an outer lock, enforce proper ordering */ > > + down_write(&dev_addr_sem); > > + netdev_lock_ops(dev); > > + > > len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, > > sizeof(*sa)); > > sa = kmalloc(len, GFP_KERNEL); > > if (!sa) { > > + up_write(&dev_addr_sem); > > err = -ENOMEM; > > goto errout; > > } > > sa->sa_family = dev->type; > > memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), > > dev->addr_len); > > Can we move down_write() and netdev_lock_ops() here ? Should be doable, yes, will also remove that up_write from the !sa error condition. Will do, thanks for the review!
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 42c75cb028e7..2bf1f914f61a 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -4198,8 +4198,6 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa, struct netlink_ext_ack *extack); int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, struct netlink_ext_ack *extack); -int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, - struct netlink_ext_ack *extack); int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, struct netlink_ext_ack *extack); int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name); diff --git a/net/core/dev.c b/net/core/dev.c index 5a64389461e2..66290c159ad8 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9592,17 +9592,6 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa, DECLARE_RWSEM(dev_addr_sem); -int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, - struct netlink_ext_ack *extack) -{ - int ret; - - down_write(&dev_addr_sem); - ret = netif_set_mac_address(dev, sa, extack); - up_write(&dev_addr_sem); - return ret; -} - int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name) { size_t size = sizeof(sa->sa_data_min); diff --git a/net/core/dev_api.c b/net/core/dev_api.c index 2e17548af685..8dbc60612100 100644 --- a/net/core/dev_api.c +++ b/net/core/dev_api.c @@ -89,9 +89,11 @@ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, { int ret; + down_write(&dev_addr_sem); netdev_lock_ops(dev); - ret = netif_set_mac_address_user(dev, sa, extack); + ret = netif_set_mac_address(dev, sa, extack); netdev_unlock_ops(dev); + up_write(&dev_addr_sem); return ret; } diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 9355058bf996..c9d44dad203d 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3080,21 +3080,32 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, struct sockaddr *sa; int len; + netdev_unlock_ops(dev); + + /* dev_addr_sem is an outer lock, enforce proper ordering */ + down_write(&dev_addr_sem); + netdev_lock_ops(dev); + len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len, sizeof(*sa)); sa = kmalloc(len, GFP_KERNEL); if (!sa) { + up_write(&dev_addr_sem); err = -ENOMEM; goto errout; } sa->sa_family = dev->type; memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), dev->addr_len); - err = netif_set_mac_address_user(dev, sa, extack); + err = netif_set_mac_address(dev, sa, extack); kfree(sa); - if (err) + if (err) { + up_write(&dev_addr_sem); goto errout; + } status |= DO_SETLINK_MODIFIED; + + up_write(&dev_addr_sem); } if (tb[IFLA_MTU]) {
Lockdep complains about circular lock in 1 -> 2 -> 3 (see below). Change the lock ordering to be: - rtnl_lock - dev_addr_sem - netdev_ops (only for lower devices!) - team_lock (or other per-upper device lock) 1. rtnl_lock -> netdev_ops -> dev_addr_sem rtnl_setlink rtnl_lock do_setlink IFLA_ADDRESS on lower netdev_ops dev_addr_sem 2. rtnl_lock -> team_lock -> netdev_ops rtnl_newlink rtnl_lock do_setlink IFLA_MASTER on lower do_set_master team_add_slave team_lock team_port_add dev_set_mtu netdev_ops 3. rtnl_lock -> dev_addr_sem -> team_lock rtnl_newlink rtnl_lock do_setlink IFLA_ADDRESS on upper dev_addr_sem netif_set_mac_address team_set_mac_address team_lock 4. rtnl_lock -> netdev_ops -> dev_addr_sem rtnl_lock dev_ifsioc dev_set_mac_address_user __tun_chr_ioctl rtnl_lock dev_set_mac_address_user tap_ioctl rtnl_lock dev_set_mac_address_user dev_set_mac_address_user netdev_lock_ops netif_set_mac_address_user dev_addr_sem Cc: Kohei Enju <enjuk@amazon.com> Fixes: df43d8bf1031 ("net: replace dev_addr_sem with netdev instance lock") Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- include/linux/netdevice.h | 2 -- net/core/dev.c | 11 ----------- net/core/dev_api.c | 4 +++- net/core/rtnetlink.c | 15 +++++++++++++-- 4 files changed, 16 insertions(+), 16 deletions(-)