forked from mirrors/linux
		
	net: replace dev_addr_sem with netdev instance lock
Lockdep reports possible circular dependency in [0]. Instead of fixing the ordering, replace global dev_addr_sem with netdev instance lock. Most of the paths that set/get mac are RTNL protected. Two places where it's not, convert to explicit locking: - sysfs address_show - dev_get_mac_address via dev_ioctl 0: https://netdev-3.bots.linux.dev/vmksft-forwarding-dbg/results/993321/24-router-bridge-1d-lag-sh/stderr Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> Link: https://patch.msgid.link/20250305163732.2766420-12-sdf@fomichev.me Signed-off-by: Jakub Kicinski <kuba@kernel.org>
This commit is contained in:
		
							parent
							
								
									2bcf4772e4
								
							
						
					
					
						commit
						df43d8bf10
					
				
					 9 changed files with 41 additions and 56 deletions
				
			
		|  | @ -1017,7 +1017,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, | ||||||
| 			rtnl_unlock(); | 			rtnl_unlock(); | ||||||
| 			return -ENOLINK; | 			return -ENOLINK; | ||||||
| 		} | 		} | ||||||
| 		ret = dev_set_mac_address_user(tap->dev, &sa, NULL); | 		ret = dev_set_mac_address(tap->dev, &sa, NULL); | ||||||
| 		tap_put_tap_dev(tap); | 		tap_put_tap_dev(tap); | ||||||
| 		rtnl_unlock(); | 		rtnl_unlock(); | ||||||
| 		return ret; | 		return ret; | ||||||
|  |  | ||||||
|  | @ -3175,7 +3175,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, | ||||||
| 
 | 
 | ||||||
| 	case SIOCSIFHWADDR: | 	case SIOCSIFHWADDR: | ||||||
| 		/* Set hw address */ | 		/* Set hw address */ | ||||||
| 		ret = dev_set_mac_address_user(tun->dev, &ifr.ifr_hwaddr, NULL); | 		ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr, NULL); | ||||||
| 		break; | 		break; | ||||||
| 
 | 
 | ||||||
| 	case TUNGETSNDBUF: | 	case TUNGETSNDBUF: | ||||||
|  |  | ||||||
|  | @ -2492,7 +2492,7 @@ struct net_device { | ||||||
| 	 * | 	 * | ||||||
| 	 * Protects: | 	 * Protects: | ||||||
| 	 *	@gro_flush_timeout, @napi_defer_hard_irqs, @napi_list, | 	 *	@gro_flush_timeout, @napi_defer_hard_irqs, @napi_list, | ||||||
| 	 *	@net_shaper_hierarchy, @reg_state, @threaded | 	 *	@net_shaper_hierarchy, @reg_state, @threaded, @dev_addr | ||||||
| 	 * | 	 * | ||||||
| 	 * Partially protects (writers must hold both @lock and rtnl_lock): | 	 * Partially protects (writers must hold both @lock and rtnl_lock): | ||||||
| 	 *	@up | 	 *	@up | ||||||
|  | @ -4262,10 +4262,6 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa, | ||||||
| 			  struct netlink_ext_ack *extack); | 			  struct netlink_ext_ack *extack); | ||||||
| int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, | int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, | ||||||
| 			struct netlink_ext_ack *extack); | 			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); | int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name); | ||||||
| int dev_get_port_parent_id(struct net_device *dev, | int dev_get_port_parent_id(struct net_device *dev, | ||||||
| 			   struct netdev_phys_item_id *ppid, bool recurse); | 			   struct netdev_phys_item_id *ppid, bool recurse); | ||||||
|  |  | ||||||
|  | @ -1058,6 +1058,28 @@ struct net_device *netdev_get_by_index_lock(struct net *net, int ifindex) | ||||||
| 	return __netdev_put_lock(dev); | 	return __netdev_put_lock(dev); | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  | /**
 | ||||||
|  |  * netdev_get_by_name_lock() - find a device by its name | ||||||
|  |  * @net: the applicable net namespace | ||||||
|  |  * @name: name of device | ||||||
|  |  * | ||||||
|  |  * Search for an interface by name. If a valid device | ||||||
|  |  * with @name is found it will be returned with netdev->lock held. | ||||||
|  |  * netdev_unlock() must be called to release it. | ||||||
|  |  * | ||||||
|  |  * Return: pointer to a device with lock held, NULL if not found. | ||||||
|  |  */ | ||||||
|  | struct net_device *netdev_get_by_name_lock(struct net *net, const char *name) | ||||||
|  | { | ||||||
|  | 	struct net_device *dev; | ||||||
|  | 
 | ||||||
|  | 	dev = dev_get_by_name(net, name); | ||||||
|  | 	if (!dev) | ||||||
|  | 		return NULL; | ||||||
|  | 
 | ||||||
|  | 	return __netdev_put_lock(dev); | ||||||
|  | } | ||||||
|  | 
 | ||||||
| struct net_device * | struct net_device * | ||||||
| netdev_xa_find_lock(struct net *net, struct net_device *dev, | netdev_xa_find_lock(struct net *net, struct net_device *dev, | ||||||
| 		    unsigned long *index) | 		    unsigned long *index) | ||||||
|  | @ -9589,44 +9611,24 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa, | ||||||
| 	return 0; | 	return 0; | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| 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) | int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name) | ||||||
| { | { | ||||||
| 	size_t size = sizeof(sa->sa_data_min); | 	size_t size = sizeof(sa->sa_data_min); | ||||||
| 	struct net_device *dev; | 	struct net_device *dev; | ||||||
| 	int ret = 0; |  | ||||||
| 
 | 
 | ||||||
| 	down_read(&dev_addr_sem); | 	dev = netdev_get_by_name_lock(net, dev_name); | ||||||
| 	rcu_read_lock(); | 	if (!dev) | ||||||
|  | 		return -ENODEV; | ||||||
| 
 | 
 | ||||||
| 	dev = dev_get_by_name_rcu(net, dev_name); |  | ||||||
| 	if (!dev) { |  | ||||||
| 		ret = -ENODEV; |  | ||||||
| 		goto unlock; |  | ||||||
| 	} |  | ||||||
| 	if (!dev->addr_len) | 	if (!dev->addr_len) | ||||||
| 		memset(sa->sa_data, 0, size); | 		memset(sa->sa_data, 0, size); | ||||||
| 	else | 	else | ||||||
| 		memcpy(sa->sa_data, dev->dev_addr, | 		memcpy(sa->sa_data, dev->dev_addr, | ||||||
| 		       min_t(size_t, size, dev->addr_len)); | 		       min_t(size_t, size, dev->addr_len)); | ||||||
| 	sa->sa_family = dev->type; | 	sa->sa_family = dev->type; | ||||||
|  | 	netdev_unlock(dev); | ||||||
| 
 | 
 | ||||||
| unlock: | 	return 0; | ||||||
| 	rcu_read_unlock(); |  | ||||||
| 	up_read(&dev_addr_sem); |  | ||||||
| 	return ret; |  | ||||||
| } | } | ||||||
| EXPORT_SYMBOL(dev_get_mac_address); | EXPORT_SYMBOL(dev_get_mac_address); | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
|  | @ -28,6 +28,7 @@ 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 *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_get_by_index_lock(struct net *net, int ifindex); | ||||||
|  | struct net_device *netdev_get_by_name_lock(struct net *net, const char *name); | ||||||
| struct net_device *__netdev_put_lock(struct net_device *dev); | struct net_device *__netdev_put_lock(struct net_device *dev); | ||||||
| struct net_device * | struct net_device * | ||||||
| netdev_xa_find_lock(struct net *net, struct net_device *dev, | netdev_xa_find_lock(struct net *net, struct net_device *dev, | ||||||
|  | @ -69,8 +70,6 @@ extern int		weight_p; | ||||||
| extern int		dev_weight_rx_bias; | extern int		dev_weight_rx_bias; | ||||||
| extern int		dev_weight_tx_bias; | extern int		dev_weight_tx_bias; | ||||||
| 
 | 
 | ||||||
| extern struct rw_semaphore dev_addr_sem; |  | ||||||
| 
 |  | ||||||
| /* rtnl helpers */ | /* rtnl helpers */ | ||||||
| extern struct list_head net_todo_list; | extern struct list_head net_todo_list; | ||||||
| void netdev_run_todo(void); | void netdev_run_todo(void); | ||||||
|  |  | ||||||
|  | @ -82,19 +82,6 @@ void dev_set_group(struct net_device *dev, int new_group) | ||||||
| 	netdev_unlock_ops(dev); | 	netdev_unlock_ops(dev); | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, |  | ||||||
| 			     struct netlink_ext_ack *extack) |  | ||||||
| { |  | ||||||
| 	int ret; |  | ||||||
| 
 |  | ||||||
| 	netdev_lock_ops(dev); |  | ||||||
| 	ret = netif_set_mac_address_user(dev, sa, extack); |  | ||||||
| 	netdev_unlock_ops(dev); |  | ||||||
| 
 |  | ||||||
| 	return ret; |  | ||||||
| } |  | ||||||
| EXPORT_SYMBOL(dev_set_mac_address_user); |  | ||||||
| 
 |  | ||||||
| /**
 | /**
 | ||||||
|  * dev_change_net_namespace() - move device to different nethost namespace |  * dev_change_net_namespace() - move device to different nethost namespace | ||||||
|  * @dev: device |  * @dev: device | ||||||
|  | @ -310,9 +297,9 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, | ||||||
| { | { | ||||||
| 	int ret; | 	int ret; | ||||||
| 
 | 
 | ||||||
| 	netdev_lock_ops(dev); | 	netdev_lock(dev); | ||||||
| 	ret = netif_set_mac_address(dev, sa, extack); | 	ret = netif_set_mac_address(dev, sa, extack); | ||||||
| 	netdev_unlock_ops(dev); | 	netdev_unlock(dev); | ||||||
| 
 | 
 | ||||||
| 	return ret; | 	return ret; | ||||||
| } | } | ||||||
|  |  | ||||||
|  | @ -574,7 +574,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, | ||||||
| 	case SIOCSIFHWADDR: | 	case SIOCSIFHWADDR: | ||||||
| 		if (dev->addr_len > sizeof(struct sockaddr)) | 		if (dev->addr_len > sizeof(struct sockaddr)) | ||||||
| 			return -EINVAL; | 			return -EINVAL; | ||||||
| 		return dev_set_mac_address_user(dev, &ifr->ifr_hwaddr, NULL); | 		return dev_set_mac_address(dev, &ifr->ifr_hwaddr, NULL); | ||||||
| 
 | 
 | ||||||
| 	case SIOCSIFHWBROADCAST: | 	case SIOCSIFHWBROADCAST: | ||||||
| 		if (ifr->ifr_hwaddr.sa_family != dev->type) | 		if (ifr->ifr_hwaddr.sa_family != dev->type) | ||||||
|  |  | ||||||
|  | @ -262,14 +262,11 @@ static ssize_t address_show(struct device *dev, struct device_attribute *attr, | ||||||
| 	struct net_device *ndev = to_net_dev(dev); | 	struct net_device *ndev = to_net_dev(dev); | ||||||
| 	ssize_t ret = -EINVAL; | 	ssize_t ret = -EINVAL; | ||||||
| 
 | 
 | ||||||
| 	down_read(&dev_addr_sem); | 	netdev_lock(ndev); | ||||||
| 
 |  | ||||||
| 	rcu_read_lock(); |  | ||||||
| 	if (dev_isalive(ndev)) | 	if (dev_isalive(ndev)) | ||||||
| 		ret = sysfs_format_mac(buf, ndev->dev_addr, ndev->addr_len); | 		ret = sysfs_format_mac(buf, ndev->dev_addr, ndev->addr_len); | ||||||
| 	rcu_read_unlock(); | 	netdev_unlock(ndev); | ||||||
| 
 | 
 | ||||||
| 	up_read(&dev_addr_sem); |  | ||||||
| 	return ret; | 	return ret; | ||||||
| } | } | ||||||
| static DEVICE_ATTR_RO(address); | static DEVICE_ATTR_RO(address); | ||||||
|  |  | ||||||
|  | @ -3089,7 +3089,11 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev, | ||||||
| 		sa->sa_family = dev->type; | 		sa->sa_family = dev->type; | ||||||
| 		memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), | 		memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]), | ||||||
| 		       dev->addr_len); | 		       dev->addr_len); | ||||||
| 		err = netif_set_mac_address_user(dev, sa, extack); | 		if (!netdev_need_ops_lock(dev)) | ||||||
|  | 			netdev_lock(dev); | ||||||
|  | 		err = netif_set_mac_address(dev, sa, extack); | ||||||
|  | 		if (!netdev_need_ops_lock(dev)) | ||||||
|  | 			netdev_unlock(dev); | ||||||
| 		kfree(sa); | 		kfree(sa); | ||||||
| 		if (err) | 		if (err) | ||||||
| 			goto errout; | 			goto errout; | ||||||
|  |  | ||||||
		Loading…
	
		Reference in a new issue
	
	 Stanislav Fomichev
						Stanislav Fomichev