Re: [Patch net] net: fix dev_ifsioc_locked() race condition
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-01-28 20:56:43
On Sat, 23 Jan 2021 17:30:49 -0800 Cong Wang wrote:
From: Cong Wang <redacted>
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.
Fixes: 3710becf8a58 ("net: RCU locking for simple ioctl()")
Reported-by: "Gong, Sishuai" <redacted>
Cc: Eric Dumazet <redacted>
Signed-off-by: Cong Wang <redacted>The addition of the write lock scares me a little for a fix, there's a lot of code which can potentially run under the callbacks and notifiers there. What about using a seqlock?
quoted hunk ↗ jump to hunk
+static DECLARE_RWSEM(dev_addr_sem); + /** * dev_set_mac_address - Change Media Access Control Address * @dev: device@@ -8729,19 +8731,50 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, return -EINVAL; if (!netif_device_present(dev)) return -ENODEV; + + down_write(&dev_addr_sem); err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack); if (err) - return err; + goto out; err = ops->ndo_set_mac_address(dev, sa); if (err) - return err; + goto out; dev->addr_assign_type = NET_ADDR_SET; call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); add_device_randomness(dev->dev_addr, dev->addr_len); - return 0; +out: + up_write(&dev_addr_sem); + return err; }