lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Mar 2022 22:54:38 +0100 From: Johannes Berg <johannes@...solutions.net> To: William McVicker <willmcvicker@...gle.com> Cc: Jakub Kicinski <kuba@...nel.org>, linux-wireless@...r.kernel.org, Marek Szyprowski <m.szyprowski@...sung.com>, Kalle Valo <kvalo@...eaurora.org>, "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org, Amitkumar Karwar <amitkarwar@...il.com>, Ganapathi Bhat <ganapathi.bhat@....com>, Xinming Hu <huxinming820@...il.com>, kernel-team@...roid.com, Paolo Abeni <pabeni@...hat.com> Subject: Re: [BUG] deadlock in nl80211_vendor_cmd On Fri, 2022-03-25 at 22:16 +0100, Johannes Berg wrote: > > > Thread 1 Thread 2 > > nl80211_pre_doit(): > > rtnl_lock() > > wiphy_lock() nl80211_pre_doit(): > > rtnl_lock() // blocked by Thread 1 > > nl80211_vendor_cmd(): > > doit() > > cfg80211_unregister_netdevice() > > rtnl_unlock(): > > netdev_run_todo(): > > __rtnl_unlock() > > <got RTNL lock> > > wiphy_lock() // blocked by Thread 1 > > rtnl_lock(); // DEADLOCK > > nl80211_post_doit(): > > wiphy_unlock(); > > > Right, this is what I had discussed in my other mails. > > Basically, you're actually doing (some form of) unregister_netdevice() > before rtnl_unlock(). > > Clearly this isn't possible in cfg80211 itself. > > However, I couldn't entirely discount the possibility that this is > possible: > > Thread 1 Thread 2 > rtnl_lock() > unregister_netdevice() > __rtnl_unlock() > rtnl_lock() > wiphy_lock() > netdev_run_todo() > __rtnl_unlock() > // list not empty now > // because of thread 2 rtnl_lock() > rtnl_lock() > wiphy_lock() > > ** DEADLOCK ** > > > Given my other discussion with Jakub though, it seems that we can indeed > make sure that this cannot happen, and then this scenario is impossible > without the unregistration you're doing. > I just sent a patch for this then, forgot to CC everyone: https://lore.kernel.org/r/20220325225055.37e89a72f814.Ic73d206e217db20fd22dcec14fe5442ca732804b@changeid But basically it changes nothing, just adds a WARN_ON with documentation ensuring that the invariant never breaks, i.e. that Thread 2 can't happen. And maybe I should've written that with 3 Threads, but the setup of unregister_netdevice()/__rtnl_unlock() could happen anywhere in the system anyway. johannes
Powered by blists - more mailing lists