[PATCH 1/2] cfg80211: fix dead lock for nl80211_new_interface()

[PATCH 1/2] cfg80211: fix dead lock for nl80211_new_interface()

[Aran Dalton]

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 <arda@allwinnertech.com>
---
 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

[PATCH 2/2] cfg80211: fix dead lock for nl80211_del_interface()

[Aran Dalton]

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 <arda@allwinnertech.com>
---
 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

Re: [PATCH 1/2] cfg80211: fix dead lock for nl80211_new_interface()

[Jeff Johnson]

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 <arda@allwinnertech.com>
> ---
>   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;
>   }

Re: [PATCH 1/2] cfg80211: fix dead lock for nl80211_new_interface()

[Johannes Berg]

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