| Message ID | 20210131022755.106005-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: fix dev_ifsioc_locked() race condition | expand |
On Sat, 30 Jan 2021 18:27:55 -0800 Cong Wang wrote: > From: Cong Wang <cong.wang@bytedance.com> > > dev_ifsioc_locked() is called with only RCU read lock, so when > there is a parallel writer changing the mac address, it could > get a partially updated mac address, as shown below: > > Thread 1 Thread 2 > // eth_commit_mac_addr_change() > memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN); > // dev_ifsioc_locked() > memcpy(ifr->ifr_hwaddr.sa_data, > dev->dev_addr,...); > > Close this race condition by guarding them with a RW semaphore, > like netdev_get_name(). The writers take RTNL anyway, so this > will not affect the slow path. To avoid bothering existing > dev_set_mac_address() callers in drivers, introduce a new wrapper > just for user-facing callers in ioctl and rtnetlink. Some of the drivers need to be update, tho, right? At a quick look at least bond and tun seem to be making calls to dev_set_mac_address() on IOCTL paths. > Fixes: 3710becf8a58 ("net: RCU locking for simple ioctl()") > Reported-by: "Gong, Sishuai" <sishuai@purdue.edu> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > --- > include/linux/netdevice.h | 3 +++ > net/core/dev.c | 40 +++++++++++++++++++++++++++++++++++++++ > net/core/dev_ioctl.c | 20 +++++++------------- > net/core/rtnetlink.c | 2 +- > 4 files changed, 51 insertions(+), 14 deletions(-)
On Mon, Feb 1, 2021 at 7:40 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 30 Jan 2021 18:27:55 -0800 Cong Wang wrote: > > From: Cong Wang <cong.wang@bytedance.com> > > > > dev_ifsioc_locked() is called with only RCU read lock, so when > > there is a parallel writer changing the mac address, it could > > get a partially updated mac address, as shown below: > > > > Thread 1 Thread 2 > > // eth_commit_mac_addr_change() > > memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN); > > // dev_ifsioc_locked() > > memcpy(ifr->ifr_hwaddr.sa_data, > > dev->dev_addr,...); > > > > Close this race condition by guarding them with a RW semaphore, > > like netdev_get_name(). The writers take RTNL anyway, so this > > will not affect the slow path. To avoid bothering existing > > dev_set_mac_address() callers in drivers, introduce a new wrapper > > just for user-facing callers in ioctl and rtnetlink. > > Some of the drivers need to be update, tho, right? At a quick look at > least bond and tun seem to be making calls to dev_set_mac_address() > on IOCTL paths. Ah, good catch! Clearly I missed those special IOCTL's. Thanks,
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index e9e7ada07ea1..8f13d3177130 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3907,6 +3907,9 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr, struct netlink_ext_ack *extack); int dev_set_mac_address(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); int dev_change_carrier(struct net_device *, bool new_carrier); int dev_get_phys_port_id(struct net_device *dev, struct netdev_phys_item_id *ppid); diff --git a/net/core/dev.c b/net/core/dev.c index aae116d059da..ecc7914784d2 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -8773,6 +8773,46 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, } EXPORT_SYMBOL(dev_set_mac_address); +static DECLARE_RWSEM(dev_addr_sem); + +int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, + struct netlink_ext_ack *extack) +{ + int ret; + + down_write(&dev_addr_sem); + ret = dev_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); + struct net_device *dev; + int ret = 0; + + down_read(&dev_addr_sem); + rcu_read_lock(); + + dev = dev_get_by_name_rcu(net, dev_name); + if (!dev) { + ret = -ENODEV; + goto unlock; + } + if (!dev->addr_len) + memset(sa->sa_data, 0, size); + else + memcpy(sa->sa_data, dev->dev_addr, + min_t(size_t, size, dev->addr_len)); + sa->sa_family = dev->type; + +unlock: + rcu_read_unlock(); + up_read(&dev_addr_sem); + return ret; +} + /** * dev_change_carrier - Change device carrier * @dev: device diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index db8a0ff86f36..478d032f34ac 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -123,17 +123,6 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm ifr->ifr_mtu = dev->mtu; return 0; - case SIOCGIFHWADDR: - if (!dev->addr_len) - memset(ifr->ifr_hwaddr.sa_data, 0, - sizeof(ifr->ifr_hwaddr.sa_data)); - else - memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr, - min(sizeof(ifr->ifr_hwaddr.sa_data), - (size_t)dev->addr_len)); - ifr->ifr_hwaddr.sa_family = dev->type; - return 0; - case SIOCGIFSLAVE: err = -EINVAL; break; @@ -274,7 +263,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd) case SIOCSIFHWADDR: if (dev->addr_len > sizeof(struct sockaddr)) return -EINVAL; - return dev_set_mac_address(dev, &ifr->ifr_hwaddr, NULL); + return dev_set_mac_address_user(dev, &ifr->ifr_hwaddr, NULL); case SIOCSIFHWBROADCAST: if (ifr->ifr_hwaddr.sa_family != dev->type) @@ -418,6 +407,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, bool *need_c */ switch (cmd) { + case SIOCGIFHWADDR: + dev_load(net, ifr->ifr_name); + ret = dev_get_mac_address(&ifr->ifr_hwaddr, net, ifr->ifr_name); + if (colon) + *colon = ':'; + return ret; /* * These ioctl calls: * - can be done by all. @@ -427,7 +422,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, bool *need_c case SIOCGIFFLAGS: case SIOCGIFMETRIC: case SIOCGIFMTU: - case SIOCGIFHWADDR: case SIOCGIFSLAVE: case SIOCGIFMAP: case SIOCGIFINDEX: diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index c313aaf2bce1..0edc0b2baaa4 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2660,7 +2660,7 @@ static int do_setlink(const struct sk_buff *skb, sa->sa_family = dev->type; memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), dev->addr_len); - err = dev_set_mac_address(dev, sa, extack); + err = dev_set_mac_address_user(dev, sa, extack); kfree(sa); if (err) goto errout;