mirror of
https://github.com/torvalds/linux.git
synced 2025-08-15 14:11:42 +02:00
net: Remove RTNL dance for SIOCBRADDIF and SIOCBRDELIF.
SIOCBRDELIF is passed to dev_ioctl() first and later forwarded to
br_ioctl_call(), which causes unnecessary RTNL dance and the splat
below [0] under RTNL pressure.
Let's say Thread A is trying to detach a device from a bridge and
Thread B is trying to remove the bridge.
In dev_ioctl(), Thread A bumps the bridge device's refcnt by
netdev_hold() and releases RTNL because the following br_ioctl_call()
also re-acquires RTNL.
In the race window, Thread B could acquire RTNL and try to remove
the bridge device. Then, rtnl_unlock() by Thread B will release RTNL
and wait for netdev_put() by Thread A.
Thread A, however, must hold RTNL after the unlock in dev_ifsioc(),
which may take long under RTNL pressure, resulting in the splat by
Thread B.
Thread A (SIOCBRDELIF) Thread B (SIOCBRDELBR)
---------------------- ----------------------
sock_ioctl sock_ioctl
`- sock_do_ioctl `- br_ioctl_call
`- dev_ioctl `- br_ioctl_stub
|- rtnl_lock |
|- dev_ifsioc '
' |- dev = __dev_get_by_name(...)
|- netdev_hold(dev, ...) .
/ |- rtnl_unlock ------. |
| |- br_ioctl_call `---> |- rtnl_lock
Race | | `- br_ioctl_stub |- br_del_bridge
Window | | | |- dev = __dev_get_by_name(...)
| | | May take long | `- br_dev_delete(dev, ...)
| | | under RTNL pressure | `- unregister_netdevice_queue(dev, ...)
| | | | `- rtnl_unlock
\ | |- rtnl_lock <-' `- netdev_run_todo
| |- ... `- netdev_run_todo
| `- rtnl_unlock |- __rtnl_unlock
| |- netdev_wait_allrefs_any
|- netdev_put(dev, ...) <----------------'
Wait refcnt decrement
and log splat below
To avoid blocking SIOCBRDELBR unnecessarily, let's not call
dev_ioctl() for SIOCBRADDIF and SIOCBRDELIF.
In the dev_ioctl() path, we do the following:
1. Copy struct ifreq by get_user_ifreq in sock_do_ioctl()
2. Check CAP_NET_ADMIN in dev_ioctl()
3. Call dev_load() in dev_ioctl()
4. Fetch the master dev from ifr.ifr_name in dev_ifsioc()
3. can be done by request_module() in br_ioctl_call(), so we move
1., 2., and 4. to br_ioctl_stub().
Note that 2. is also checked later in add_del_if(), but it's better
performed before RTNL.
SIOCBRADDIF and SIOCBRDELIF have been processed in dev_ioctl() since
the pre-git era, and there seems to be no specific reason to process
them there.
[0]:
unregister_netdevice: waiting for wpan3 to become free. Usage count = 2
ref_tracker: wpan3@ffff8880662d8608 has 1/1 users at
__netdev_tracker_alloc include/linux/netdevice.h:4282 [inline]
netdev_hold include/linux/netdevice.h:4311 [inline]
dev_ifsioc+0xc6a/0x1160 net/core/dev_ioctl.c:624
dev_ioctl+0x255/0x10c0 net/core/dev_ioctl.c:826
sock_do_ioctl+0x1ca/0x260 net/socket.c:1213
sock_ioctl+0x23a/0x6c0 net/socket.c:1318
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:906 [inline]
__se_sys_ioctl fs/ioctl.c:892 [inline]
__x64_sys_ioctl+0x1a4/0x210 fs/ioctl.c:892
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcb/0x250 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Fixes: 893b195875 ("net: bridge: fix ioctl locking")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Reported-by: yan kang <kangyan91@outlook.com>
Reported-by: yue sun <samsun1006219@gmail.com>
Closes: https://lore.kernel.org/netdev/SY8P300MB0421225D54EB92762AE8F0F2A1D32@SY8P300MB0421.AUSP300.PROD.OUTLOOK.COM/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Link: https://patch.msgid.link/20250316192851.19781-1-kuniyu@amazon.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
This commit is contained in:
Kuniyuki Iwashima
Paolo Abeni
parent
919f9f497d
commit
ed3ba9b6e2
5 changed files with 45 additions and 38 deletions
|
|
@ -65,11 +65,9 @@ struct br_ip_list {
|
|||
#define BR_DEFAULT_AGEING_TIME (300 * HZ)
|
||||
|
||||
struct net_bridge;
|
||||
void brioctl_set(int (*hook)(struct net *net, struct net_bridge *br,
|
||||
unsigned int cmd, struct ifreq *ifr,
|
||||
void brioctl_set(int (*hook)(struct net *net, unsigned int cmd,
|
||||
void __user *uarg));
|
||||
int br_ioctl_call(struct net *net, struct net_bridge *br, unsigned int cmd,
|
||||
struct ifreq *ifr, void __user *uarg);
|
||||
int br_ioctl_call(struct net *net, unsigned int cmd, void __user *uarg);
|
||||
|
||||
#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_IGMP_SNOOPING)
|
||||
int br_multicast_list_adjacent(struct net_device *dev,
|
||||
|
|
|
|||
|
|
@ -394,10 +394,26 @@ static int old_deviceless(struct net *net, void __user *data)
|
|||
return -EOPNOTSUPP;
|
||||
}
|
||||
|
||||
int br_ioctl_stub(struct net *net, struct net_bridge *br, unsigned int cmd,
|
||||
struct ifreq *ifr, void __user *uarg)
|
||||
int br_ioctl_stub(struct net *net, unsigned int cmd, void __user *uarg)
|
||||
{
|
||||
int ret = -EOPNOTSUPP;
|
||||
struct ifreq ifr;
|
||||
|
||||
if (cmd == SIOCBRADDIF || cmd == SIOCBRDELIF) {
|
||||
void __user *data;
|
||||
char *colon;
|
||||
|
||||
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
|
||||
return -EPERM;
|
||||
|
||||
if (get_user_ifreq(&ifr, &data, uarg))
|
||||
return -EFAULT;
|
||||
|
||||
ifr.ifr_name[IFNAMSIZ - 1] = 0;
|
||||
colon = strchr(ifr.ifr_name, ':');
|
||||
if (colon)
|
||||
*colon = 0;
|
||||
}
|
||||
|
||||
rtnl_lock();
|
||||
|
||||
|
|
@ -430,7 +446,21 @@ int br_ioctl_stub(struct net *net, struct net_bridge *br, unsigned int cmd,
|
|||
break;
|
||||
case SIOCBRADDIF:
|
||||
case SIOCBRDELIF:
|
||||
ret = add_del_if(br, ifr->ifr_ifindex, cmd == SIOCBRADDIF);
|
||||
{
|
||||
struct net_device *dev;
|
||||
|
||||
dev = __dev_get_by_name(net, ifr.ifr_name);
|
||||
if (!dev || !netif_device_present(dev)) {
|
||||
ret = -ENODEV;
|
||||
break;
|
||||
}
|
||||
if (!netif_is_bridge_master(dev)) {
|
||||
ret = -EOPNOTSUPP;
|
||||
break;
|
||||
}
|
||||
|
||||
ret = add_del_if(netdev_priv(dev), ifr.ifr_ifindex, cmd == SIOCBRADDIF);
|
||||
}
|
||||
break;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -949,8 +949,7 @@ br_port_get_check_rtnl(const struct net_device *dev)
|
|||
/* br_ioctl.c */
|
||||
int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
|
||||
void __user *data, int cmd);
|
||||
int br_ioctl_stub(struct net *net, struct net_bridge *br, unsigned int cmd,
|
||||
struct ifreq *ifr, void __user *uarg);
|
||||
int br_ioctl_stub(struct net *net, unsigned int cmd, void __user *uarg);
|
||||
|
||||
/* br_multicast.c */
|
||||
#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
|
||||
|
|
|
|||
|
|
@ -551,7 +551,6 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
|
|||
int err;
|
||||
struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);
|
||||
const struct net_device_ops *ops;
|
||||
netdevice_tracker dev_tracker;
|
||||
|
||||
if (!dev)
|
||||
return -ENODEV;
|
||||
|
|
@ -614,22 +613,6 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
|
|||
case SIOCWANDEV:
|
||||
return dev_siocwandev(dev, &ifr->ifr_settings);
|
||||
|
||||
case SIOCBRADDIF:
|
||||
case SIOCBRDELIF:
|
||||
if (!netif_device_present(dev))
|
||||
return -ENODEV;
|
||||
if (!netif_is_bridge_master(dev))
|
||||
return -EOPNOTSUPP;
|
||||
|
||||
netdev_hold(dev, &dev_tracker, GFP_KERNEL);
|
||||
rtnl_net_unlock(net);
|
||||
|
||||
err = br_ioctl_call(net, netdev_priv(dev), cmd, ifr, NULL);
|
||||
|
||||
netdev_put(dev, &dev_tracker);
|
||||
rtnl_net_lock(net);
|
||||
return err;
|
||||
|
||||
case SIOCDEVPRIVATE ... SIOCDEVPRIVATE + 15:
|
||||
return dev_siocdevprivate(dev, ifr, data, cmd);
|
||||
|
||||
|
|
@ -812,8 +795,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
|
|||
case SIOCBONDRELEASE:
|
||||
case SIOCBONDSETHWADDR:
|
||||
case SIOCBONDCHANGEACTIVE:
|
||||
case SIOCBRADDIF:
|
||||
case SIOCBRDELIF:
|
||||
case SIOCSHWTSTAMP:
|
||||
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
|
||||
return -EPERM;
|
||||
|
|
|
|||
19
net/socket.c
19
net/socket.c
|
|
@ -1145,12 +1145,10 @@ static ssize_t sock_write_iter(struct kiocb *iocb, struct iov_iter *from)
|
|||
*/
|
||||
|
||||
static DEFINE_MUTEX(br_ioctl_mutex);
|
||||
static int (*br_ioctl_hook)(struct net *net, struct net_bridge *br,
|
||||
unsigned int cmd, struct ifreq *ifr,
|
||||
static int (*br_ioctl_hook)(struct net *net, unsigned int cmd,
|
||||
void __user *uarg);
|
||||
|
||||
void brioctl_set(int (*hook)(struct net *net, struct net_bridge *br,
|
||||
unsigned int cmd, struct ifreq *ifr,
|
||||
void brioctl_set(int (*hook)(struct net *net, unsigned int cmd,
|
||||
void __user *uarg))
|
||||
{
|
||||
mutex_lock(&br_ioctl_mutex);
|
||||
|
|
@ -1159,8 +1157,7 @@ void brioctl_set(int (*hook)(struct net *net, struct net_bridge *br,
|
|||
}
|
||||
EXPORT_SYMBOL(brioctl_set);
|
||||
|
||||
int br_ioctl_call(struct net *net, struct net_bridge *br, unsigned int cmd,
|
||||
struct ifreq *ifr, void __user *uarg)
|
||||
int br_ioctl_call(struct net *net, unsigned int cmd, void __user *uarg)
|
||||
{
|
||||
int err = -ENOPKG;
|
||||
|
||||
|
|
@ -1169,7 +1166,7 @@ int br_ioctl_call(struct net *net, struct net_bridge *br, unsigned int cmd,
|
|||
|
||||
mutex_lock(&br_ioctl_mutex);
|
||||
if (br_ioctl_hook)
|
||||
err = br_ioctl_hook(net, br, cmd, ifr, uarg);
|
||||
err = br_ioctl_hook(net, cmd, uarg);
|
||||
mutex_unlock(&br_ioctl_mutex);
|
||||
|
||||
return err;
|
||||
|
|
@ -1269,7 +1266,9 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
|
|||
case SIOCSIFBR:
|
||||
case SIOCBRADDBR:
|
||||
case SIOCBRDELBR:
|
||||
err = br_ioctl_call(net, NULL, cmd, NULL, argp);
|
||||
case SIOCBRADDIF:
|
||||
case SIOCBRDELIF:
|
||||
err = br_ioctl_call(net, cmd, argp);
|
||||
break;
|
||||
case SIOCGIFVLAN:
|
||||
case SIOCSIFVLAN:
|
||||
|
|
@ -3429,6 +3428,8 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
|
|||
case SIOCGPGRP:
|
||||
case SIOCBRADDBR:
|
||||
case SIOCBRDELBR:
|
||||
case SIOCBRADDIF:
|
||||
case SIOCBRDELIF:
|
||||
case SIOCGIFVLAN:
|
||||
case SIOCSIFVLAN:
|
||||
case SIOCGSKNS:
|
||||
|
|
@ -3468,8 +3469,6 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
|
|||
case SIOCGIFPFLAGS:
|
||||
case SIOCGIFTXQLEN:
|
||||
case SIOCSIFTXQLEN:
|
||||
case SIOCBRADDIF:
|
||||
case SIOCBRDELIF:
|
||||
case SIOCGIFNAME:
|
||||
case SIOCSIFNAME:
|
||||
case SIOCGMIIPHY:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue