Linux Ethernet Bridging - Aug 2023 - [Bridge] [PATCH] net: bridge: Fix refcnt issues in dev_ioctl

In the bug reported by Syzbot, certain bridge devices would have a
leaked reference created by race conditions in dev_ioctl, specifically,
under SIOCBRADDIF or SIOCBRDELIF operations. The reference leak would
be shown in the periodic unregister_netdevice call, which throws a
warning and cause Syzbot to report a crash. Upon inspection of the
logic in dev_ioctl, it seems the reference was introduced to ensure
proper access to the bridge device after rtnl_unlock. and the latter
function is necessary to maintain the following lock order in any
bridge related ioctl calls:

1) br_ioctl_mutex => 2) rtnl_lock

Conceptually, though, br_ioctl_mutex could be considered more specific
than rtnl_lock given their usages, hence swapping their order would be
a reasonable proposal. This patch changes all related call sites to
maintain the reversed order of the two locks:

1) rtnl_lock => 2) br_ioctl_mutex

By doing so, the extra reference introduced in dev_ioctl is no longer
needed, and hence the reference leak bug is now resolved.

Reported-by: syzbot+881d65229ca4f9ae8c84 at syzkaller.appspotmail.com
Fixes: ad2f99aedf8f ("net: bridge: move bridge ioctls out of
.ndo_do_ioctl")
Signed-off-by: Ziqi Zhao <astrajoan at yahoo.com>
---
 net/bridge/br_ioctl.c | 4 ----
 net/core/dev_ioctl.c  | 8 +-------
 net/socket.c          | 2 ++
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index f213ed108361..291dbc5d2a99 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -399,8 +399,6 @@ int br_ioctl_stub(struct net *net, struct net_bridge *br,
unsigned int cmd,
 {
 	int ret = -EOPNOTSUPP;
 
-	rtnl_lock();
-
 	switch (cmd) {
 	case SIOCGIFBR:
 	case SIOCSIFBR:
@@ -434,7 +432,5 @@ int br_ioctl_stub(struct net *net, struct net_bridge *br,
unsigned int cmd,
 		break;
 	}
 
-	rtnl_unlock();
-
 	return ret;
 }
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 3730945ee294..17df956df8cb 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -336,7 +336,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;
@@ -405,12 +404,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr,
void __user *data,
 			return -ENODEV;
 		if (!netif_is_bridge_master(dev))
 			return -EOPNOTSUPP;
-		netdev_hold(dev, &dev_tracker, GFP_KERNEL);
-		rtnl_unlock();
-		err = br_ioctl_call(net, netdev_priv(dev), cmd, ifr, NULL);
-		netdev_put(dev, &dev_tracker);
-		rtnl_lock();
-		return err;
+		return br_ioctl_call(net, netdev_priv(dev), cmd, ifr, NULL);
 
 	case SIOCDEVPRIVATE ... SIOCDEVPRIVATE + 15:
 		return dev_siocdevprivate(dev, ifr, data, cmd);
diff --git a/net/socket.c b/net/socket.c
index 2b0e54b2405c..6b7a9df9a326 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1258,7 +1258,9 @@ static long sock_ioctl(struct file *file, unsigned cmd,
unsigned long arg)
 		case SIOCSIFBR:
 		case SIOCBRADDBR:
 		case SIOCBRDELBR:
+			rtnl_lock();
 			err = br_ioctl_call(net, NULL, cmd, NULL, argp);
+			rtnl_unlock();
 			break;
 		case SIOCGIFVLAN:
 		case SIOCSIFVLAN:
-- 
2.34.1

Hi Ziqi,
On 8/19/23 11:10, Ziqi Zhao wrote:
> In the bug reported by Syzbot, certain bridge devices would have a
> leaked reference created by race conditions in dev_ioctl, specifically,
> under SIOCBRADDIF or SIOCBRDELIF operations. The reference leak would
How would it leak a reference, could you elaborate?
The reference is always taken and always released after the call.
> be shown in the periodic unregister_netdevice call, which throws a
> warning and cause Syzbot to report a crash. Upon inspection of the
If you reproduced it, is the device later removed or is it really stuck?
> logic in dev_ioctl, it seems the reference was introduced to ensure
> proper access to the bridge device after rtnl_unlock. and the latter
> function is necessary to maintain the following lock order in any
> bridge related ioctl calls:
> 
> 1) br_ioctl_mutex => 2) rtnl_lock
> 
> Conceptually, though, br_ioctl_mutex could be considered more specific
> than rtnl_lock given their usages, hence swapping their order would be
> a reasonable proposal. This patch changes all related call sites to
> maintain the reversed order of the two locks:
> 
> 1) rtnl_lock => 2) br_ioctl_mutex
> 
> By doing so, the extra reference introduced in dev_ioctl is no longer
> needed, and hence the reference leak bug is now resolved.
IIRC there was no bug, it was a false-positive. The reference is held a 
bit longer but then released, so the device is deleted later.
I might be remembering wrong, but I think I briefly looked into this 
when it was reported. If that's not the case I'd be interested to see
a new report/trace because the bug might be somewhere else.
> 
> Reported-by: syzbot+881d65229ca4f9ae8c84 at syzkaller.appspotmail.com
> Fixes: ad2f99aedf8f ("net: bridge: move bridge ioctls out of
.ndo_do_ioctl")
> Signed-off-by: Ziqi Zhao <astrajoan at yahoo.com>
> ---
>   net/bridge/br_ioctl.c | 4 ----
>   net/core/dev_ioctl.c  | 8 +-------
>   net/socket.c          | 2 ++
>   3 files changed, 3 insertions(+), 11 deletions(-)
> 
Thanks,
  Nik