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: 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>
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>

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 3ff96ae31bf6dea59ca26f09959544e5d0620a66..c5fe3b2a53e8275b6021d6ff05d5eb9849cd93c2 100644 (file)
--- 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 f213ed10836185d338060a9d02ab7a2de862e9b4..6bc0a11f2ed3e6cf4c01e3dd2d0ec78c5190bf8a 100644 (file)
--- 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 1054b8a88edc472e6faadc8b2eb0483cddf020be..d5b3c5936a79e1c6b52b51e53c34cd1af6bfa1f9 100644 (file)
--- 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 4c2098ac9d7243eded892759165e0f668741f50a..57f79f8e84665e0fe3bb43f418c01e9ee580ac14 100644 (file)
--- 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 28bae5a942341b94afec01d5f2f43f710f8ed000..38227d00d1987bb1f66dc7d4f9f792f651be0196 100644 (file)
--- 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: