| Message ID | 20250316192851.19781-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: Remove RTNL dance for SIOCBRADDIF and SIOCBRDELIF. | expand |
On Sun, Mar 16, 2025 at 12:28:37PM -0700, Kuniyuki Iwashima wrote: > 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: 893b19587534 ("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>
On 3/16/25 9:28 PM, Kuniyuki Iwashima wrote: > 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: 893b19587534 ("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> > --- > v2: > * Use if in br_ioctl_stub() > * Update diagram in commit message > > v1: https://lore.kernel.org/netdev/20250314010501.75798-1-kuniyu@amazon.com/ > --- > include/linux/if_bridge.h | 6 ++---- > net/bridge/br_ioctl.c | 36 +++++++++++++++++++++++++++++++++--- > net/bridge/br_private.h | 3 +-- > net/core/dev_ioctl.c | 19 ------------------- > net/socket.c | 19 +++++++++---------- > 5 files changed, 45 insertions(+), 38 deletions(-) > Thanks, Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h index 3ff96ae31bf6..c5fe3b2a53e8 100644 --- a/include/linux/if_bridge.h +++ b/include/linux/if_bridge.h @@ -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, diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index f213ed108361..6bc0a11f2ed3 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -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; } diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 1054b8a88edc..d5b3c5936a79 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -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 diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 4c2098ac9d72..57f79f8e8466 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -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; diff --git a/net/socket.c b/net/socket.c index 28bae5a94234..38227d00d198 100644 --- a/net/socket.c +++ b/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:
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: 893b19587534 ("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> --- v2: * Use if in br_ioctl_stub() * Update diagram in commit message v1: https://lore.kernel.org/netdev/20250314010501.75798-1-kuniyu@amazon.com/ --- include/linux/if_bridge.h | 6 ++---- net/bridge/br_ioctl.c | 36 +++++++++++++++++++++++++++++++++--- net/bridge/br_private.h | 3 +-- net/core/dev_ioctl.c | 19 ------------------- net/socket.c | 19 +++++++++---------- 5 files changed, 45 insertions(+), 38 deletions(-)