Messages in this thread | | | Date | Sat, 19 Aug 2023 12:25:15 +0300 | Subject | Re: [PATCH] net: bridge: Fix refcnt issues in dev_ioctl | From | Nikolay Aleksandrov <> |
| |
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@syzkaller.appspotmail.com > Fixes: ad2f99aedf8f ("net: bridge: move bridge ioctls out of .ndo_do_ioctl") > Signed-off-by: Ziqi Zhao <astrajoan@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
| |