cfg80211: fix locking in nl80211_set_wiphy

Luis reports that there's a circular locking dependency; this is because cfg80211_dev_rename() will acquire the cfg80211_mutex while the device mutex is held, while this normally is done the other way around. The solution is to open-code the device-getting in nl80211_set_wiphy and require holding the mutex around cfg80211_dev_rename rather than acquiring it within. Also fix a bug -- rtnl locking is expected by drivers so we need to provide it. Reported-by: Luis R. Rodriguez <[email protected]> Signed-off-by: Johannes Berg <[email protected]> Signed-off-by: John W. Linville <[email protected]>
mrcsosa · Mar 28, 2009 · 4bbf4d5 · 4bbf4d5
1 parent 3832c28
commit 4bbf4d5
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 28 deletions.
diff --git a/net/wireless/core.c b/net/wireless/core.c
@@ -87,7 +87,7 @@ struct wiphy *wiphy_idx_to_wiphy(int wiphy_idx)
 }
 
 /* requires cfg80211_mutex to be held! */
-static struct cfg80211_registered_device *
+struct cfg80211_registered_device *
 __cfg80211_drv_from_info(struct genl_info *info)
 {
 	int ifindex;
@@ -176,13 +176,14 @@ void cfg80211_put_dev(struct cfg80211_registered_device *drv)
 	mutex_unlock(&drv->mtx);
 }
 
+/* requires cfg80211_mutex to be held */
 int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 			char *newname)
 {
 	struct cfg80211_registered_device *drv;
 	int wiphy_idx, taken = -1, result, digits;
 
-	mutex_lock(&cfg80211_mutex);
+	assert_cfg80211_lock();
 
 	/* prohibit calling the thing phy%d when %d is not its number */
 	sscanf(newname, PHY_NAME "%d%n", &wiphy_idx, &taken);
@@ -195,30 +196,23 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		 * deny the name if it is phy<idx> where <idx> is printed
 		 * without leading zeroes. taken == strlen(newname) here
 		 */
-		result = -EINVAL;
 		if (taken == strlen(PHY_NAME) + digits)
-			goto out_unlock;
+			return -EINVAL;
 	}
 
 
 	/* Ignore nop renames */
-	result = 0;
 	if (strcmp(newname, dev_name(&rdev->wiphy.dev)) == 0)
-		goto out_unlock;
+		return 0;
 
 	/* Ensure another device does not already have this name. */
-	list_for_each_entry(drv, &cfg80211_drv_list, list) {
-		result = -EINVAL;
+	list_for_each_entry(drv, &cfg80211_drv_list, list)
 		if (strcmp(newname, dev_name(&drv->wiphy.dev)) == 0)
-			goto out_unlock;
-	}
+			return -EINVAL;
 
-	/* this will only check for collisions in sysfs
-	 * which is not even always compiled in.
-	 */
 	result = device_rename(&rdev->wiphy.dev, newname);
 	if (result)
-		goto out_unlock;
+		return result;
 
 	if (rdev->wiphy.debugfsdir &&
 	    !debugfs_rename(rdev->wiphy.debugfsdir->d_parent,
@@ -228,13 +222,9 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		printk(KERN_ERR "cfg80211: failed to rename debugfs dir to %s!\n",
 		       newname);
 
-	result = 0;
-out_unlock:
-	mutex_unlock(&cfg80211_mutex);
-	if (result == 0)
-		nl80211_notify_dev_rename(rdev);
+	nl80211_notify_dev_rename(rdev);
 
-	return result;
+	return 0;
 }
 
 /* exported functions */

diff --git a/net/wireless/core.h b/net/wireless/core.h
@@ -99,6 +99,9 @@ struct cfg80211_internal_bss {
 struct cfg80211_registered_device *cfg80211_drv_by_wiphy_idx(int wiphy_idx);
 int get_wiphy_idx(struct wiphy *wiphy);
 
+struct cfg80211_registered_device *
+__cfg80211_drv_from_info(struct genl_info *info);
+
 /*
  * This function returns a pointer to the driver
  * that the genl_info item that is passed refers to.

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
@@ -366,16 +366,26 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 	int result = 0, rem_txq_params = 0;
 	struct nlattr *nl_txq_params;
 
-	rdev = cfg80211_get_dev_from_info(info);
-	if (IS_ERR(rdev))
-		return PTR_ERR(rdev);
+	rtnl_lock();
+
+	mutex_lock(&cfg80211_mutex);
+
+	rdev = __cfg80211_drv_from_info(info);
+	if (IS_ERR(rdev)) {
+		result = PTR_ERR(rdev);
+		goto unlock;
+	}
 
-	if (info->attrs[NL80211_ATTR_WIPHY_NAME]) {
+	mutex_lock(&rdev->mtx);
+
+	if (info->attrs[NL80211_ATTR_WIPHY_NAME])
 		result = cfg80211_dev_rename(
 			rdev, nla_data(info->attrs[NL80211_ATTR_WIPHY_NAME]));
-		if (result)
-			goto bad_res;
-	}
+
+	mutex_unlock(&cfg80211_mutex);
+
+	if (result)
+		goto bad_res;
 
 	if (info->attrs[NL80211_ATTR_WIPHY_TXQ_PARAMS]) {
 		struct ieee80211_txq_params txq_params;
@@ -471,7 +481,9 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 
 
  bad_res:
-	cfg80211_put_dev(rdev);
+	mutex_unlock(&rdev->mtx);
+ unlock:
+	rtnl_unlock();
 	return result;
 }