Both nl80211_new_interface and cfg80211_netdev_notifier_call hold the
same wiphy_lock, then cause deadlock.
The main call stack as bellow:
nl80211_new_interface() takes wiphy_lock
-> _nl80211_new_interface:
-> rdev_add_virtual_intf
-> rdev->ops->add_virtual_intf
-> register_netdevice
-> call_netdevice_notifiers(NETDEV_REGISTER, dev);
-> call_netdevice_notifiers_extack
-> call_netdevice_notifiers_info
-> raw_notifier_call_chain
-> cfg80211_netdev_notifier_call
-> wiphy_lock(&rdev->wiphy), cfg80211_register_wdev
Fixes: ea6b2098dd02 ("cfg80211: fix locking in netlink owner interface destruction")
Signed-off-by: Aran Dalton <[email protected]>
---
net/wireless/nl80211.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2705e3ee8fc4..bdacddc3ffa3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4260,9 +4260,7 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
/* to avoid failing a new interface creation due to pending removal */
cfg80211_destroy_ifaces(rdev);
- wiphy_lock(&rdev->wiphy);
ret = _nl80211_new_interface(skb, info);
- wiphy_unlock(&rdev->wiphy);
return ret;
}
--
2.29.0
Both nl80211_del_interface and cfg80211_netdev_notifier_call hold the
same wiphy_lock, then cause deadlock.
The main call stack as bellow:
nl80211_del_interface() takes wiphy_lock
-> cfg80211_remove_virtual_intf
-> rdev_del_virtual_intf
-> rdev->ops->del_virtual_intf
-> cfg80211_unregister_netdevice
-> cfg80211_unregister_wdev
-> _cfg80211_unregister_wdev
-> unregister_netdevice
-> unregister_netdevice_queue
-> unregister_netdevice_many
-> call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
-> call_netdevice_notifiers_extack
-> call_netdevice_notifiers_info
-> raw_notifier_call_chain
-> cfg80211_netdev_notifier_call
-> wiphy_lock(&rdev->wiphy), _cfg80211_unregister_wdev
Fixes: a05829a7222e ("cfg80211: avoid holding the RTNL when calling the driver")
Signed-off-by: Aran Dalton <[email protected]>
---
net/wireless/nl80211.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index bdacddc3ffa3..664bf977b7bc 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4269,6 +4269,7 @@ static int nl80211_del_interface(struct sk_buff *skb, struct genl_info *info)
{
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct wireless_dev *wdev = info->user_ptr[1];
+ int ret;
if (!rdev->ops->del_virtual_intf)
return -EOPNOTSUPP;
@@ -4296,9 +4297,11 @@ static int nl80211_del_interface(struct sk_buff *skb, struct genl_info *info)
else
dev_close(wdev->netdev);
+ ret = cfg80211_remove_virtual_intf(rdev, wdev);
+
mutex_lock(&rdev->wiphy.mtx);
- return cfg80211_remove_virtual_intf(rdev, wdev);
+ return ret;
}
static int nl80211_set_noack_map(struct sk_buff *skb, struct genl_info *info)
--
2.29.0
On 9/21/2022 2:19 AM, Aran Dalton wrote:
> Both nl80211_new_interface and cfg80211_netdev_notifier_call hold the
> same wiphy_lock, then cause deadlock.
>
> The main call stack as bellow:
>
> nl80211_new_interface() takes wiphy_lock
> -> _nl80211_new_interface:
> -> rdev_add_virtual_intf
> -> rdev->ops->add_virtual_intf
> -> register_netdevice
> -> call_netdevice_notifiers(NETDEV_REGISTER, dev);
> -> call_netdevice_notifiers_extack
> -> call_netdevice_notifiers_info
> -> raw_notifier_call_chain
> -> cfg80211_netdev_notifier_call
> -> wiphy_lock(&rdev->wiphy), cfg80211_register_wdev
In both of your patches please describe what you are doing in the patch
to fix the problem, and in particular describe why your fix is safe.
>
> Fixes: ea6b2098dd02 ("cfg80211: fix locking in netlink owner interface destruction")
> Signed-off-by: Aran Dalton <[email protected]>
> ---
> net/wireless/nl80211.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2705e3ee8fc4..bdacddc3ffa3 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -4260,9 +4260,7 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
> /* to avoid failing a new interface creation due to pending removal */
> cfg80211_destroy_ifaces(rdev);
>
> - wiphy_lock(&rdev->wiphy);
> ret = _nl80211_new_interface(skb, info);
> - wiphy_unlock(&rdev->wiphy);
>
> return ret;
> }
On Wed, 2022-09-21 at 17:19 +0800, Aran Dalton wrote:
> Both nl80211_new_interface and cfg80211_netdev_notifier_call hold the
> same wiphy_lock, then cause deadlock.
>
> The main call stack as bellow:
>
> nl80211_new_interface() takes wiphy_lock
> -> _nl80211_new_interface:
> -> rdev_add_virtual_intf
> -> rdev->ops->add_virtual_intf
> -> register_netdevice
The bug is yours, here, you're no longer allowed to call
register_netdevice() here.
If you have an out-of-tree driver that we couldn't update when doing
tree-wide changes, you probably shouldn't assume that the bug is
upstream and send random locking patches ... :)
johannes