cfg80211: fix dangling scan request checking

My patch "cfg80211: fix deadlock" broke the code it was supposed to fix, the scan request checking. But it's not trivial to put it back the way it was, since the original patch had a deadlock. Now do it in a completely new way: queue the check off to a work struct, where we can freely lock. But that has some more complications, like needing to wait for it to be done before the wiphy/rdev can be destroyed, so some code is required to handle that. Signed-off-by: Johannes Berg <johannes@sipsolutions.net> Signed-off-by: John W. Linville <linville@tuxdriver.com>

cfg80211: fix dangling scan request checking
My patch "cfg80211: fix deadlock" broke the code it was supposed to fix, the scan request checking. But it's not trivial to put it back the way it was, since the original patch had a deadlock. Now do it in a completely new way: queue the check off to a work struct, where we can freely lock. But that has some more complications, like needing to wait for it to be done before the wiphy/rdev can be destroyed, so some code is required to handle that. Signed-off-by: Johannes Berg <johannes@sipsolutions.net> Signed-off-by: John W. Linville <linville@tuxdriver.com>
Johannes Berg · John W. Linville
1 parent 21f8a73f82
Showing 3 changed files with 67 additions and 13 deletions Side-by-side Diff
include/net/cfg80211.h
net/wireless/core.c
net/wireless/core.h
@@ -1325,6 +1325,8 @@
  
 	struct mutex mtx;
  
+	struct work_struct cleanup_work;
+
 	/* currently used for IBSS and SME - might be rearranged later */
 	u8 ssid[IEEE80211_MAX_SSID_LEN];
 	u8 ssid_len;
@@ -430,6 +430,8 @@
 	INIT_WORK(&rdev->conn_work, cfg80211_conn_work);
 	INIT_WORK(&rdev->event_work, cfg80211_event_work);
  
+	init_waitqueue_head(&rdev->dev_wait);
+
 	/*
 	 * Initialize wiphy parameters to IEEE 802.11 MIB default values.
 	 * Fragmentation and RTS threshold are disabled by default with the
  
  
  
  
  
  
@@ -574,34 +576,42 @@
 	/* protect the device list */
 	mutex_lock(&cfg80211_mutex);
  
+	wait_event(rdev->dev_wait, ({
+		int __count;
+		mutex_lock(&rdev->devlist_mtx);
+		__count = rdev->opencount;
+		mutex_unlock(&rdev->devlist_mtx);
+		__count == 0;}));
+
+	mutex_lock(&rdev->devlist_mtx);
 	BUG_ON(!list_empty(&rdev->netdev_list));
+	mutex_unlock(&rdev->devlist_mtx);
  
 	/*
+	 * First remove the hardware from everywhere, this makes
+	 * it impossible to find from userspace.
+	 */
+	cfg80211_debugfs_rdev_del(rdev);
+	list_del(&rdev->list);
+
+	/*
 	 * Try to grab rdev->mtx. If a command is still in progress,
 	 * hopefully the driver will refuse it since it's tearing
 	 * down the device already. We wait for this command to complete
 	 * before unlinking the item from the list.
 	 * Note: as codified by the BUG_ON above we cannot get here if
-	 * a virtual interface is still associated. Hence, we can only
-	 * get to lock contention here if userspace issues a command
-	 * that identified the hardware by wiphy index.
+	 * a virtual interface is still present. Hence, we can only get
+	 * to lock contention here if userspace issues a command that
+	 * identified the hardware by wiphy index.
 	 */
 	cfg80211_lock_rdev(rdev);
-
-	if (WARN_ON(rdev->scan_req)) {
-		rdev->scan_req->aborted = true;
-		___cfg80211_scan_done(rdev);
-	}
-
+	/* nothing */
 	cfg80211_unlock_rdev(rdev);
  
-	cfg80211_debugfs_rdev_del(rdev);
-
 	/* If this device got a regulatory hint tell core its
 	 * free to listen now to a new shiny device regulatory hint */
 	reg_device_remove(wiphy);
  
-	list_del(&rdev->list);
 	cfg80211_rdev_list_generation++;
 	device_del(&rdev->wiphy.dev);
 	debugfs_remove(rdev->wiphy.debugfsdir);
@@ -640,6 +650,31 @@
 }
 EXPORT_SYMBOL(wiphy_rfkill_set_hw_state);
  
+static void wdev_cleanup_work(struct work_struct *work)
+{
+	struct wireless_dev *wdev;
+	struct cfg80211_registered_device *rdev;
+
+	wdev = container_of(work, struct wireless_dev, cleanup_work);
+	rdev = wiphy_to_dev(wdev->wiphy);
+
+	cfg80211_lock_rdev(rdev);
+
+	if (WARN_ON(rdev->scan_req && rdev->scan_req->dev == wdev->netdev)) {
+		rdev->scan_req->aborted = true;
+		___cfg80211_scan_done(rdev);
+	}
+
+	cfg80211_unlock_rdev(rdev);
+
+	mutex_lock(&rdev->devlist_mtx);
+	rdev->opencount--;
+	mutex_unlock(&rdev->devlist_mtx);
+	wake_up(&rdev->dev_wait);
+
+	dev_put(wdev->netdev);
+}
+
 static int cfg80211_netdev_notifier_call(struct notifier_block * nb,
 					 unsigned long state,
 					 void *ndev)
@@ -663,6 +698,7 @@
 		 * are added with nl80211.
 		 */
 		mutex_init(&wdev->mtx);
+		INIT_WORK(&wdev->cleanup_work, wdev_cleanup_work);
 		INIT_LIST_HEAD(&wdev->event_list);
 		spin_lock_init(&wdev->event_lock);
 		mutex_lock(&rdev->devlist_mtx);
  
@@ -717,8 +753,22 @@
 		default:
 			break;
 		}
+		dev_hold(dev);
+		schedule_work(&wdev->cleanup_work);
 		break;
 	case NETDEV_UP:
+		/*
+		 * If we have a really quick DOWN/UP succession we may
+		 * have this work still pending ... cancel it and see
+		 * if it was pending, in which case we need to account
+		 * for some of the work it would have done.
+		 */
+		if (cancel_work_sync(&wdev->cleanup_work)) {
+			mutex_lock(&rdev->devlist_mtx);
+			rdev->opencount--;
+			mutex_unlock(&rdev->devlist_mtx);
+			dev_put(dev);
+		}
 #ifdef CONFIG_WIRELESS_EXT
 		cfg80211_lock_rdev(rdev);
 		mutex_lock(&rdev->devlist_mtx);
@@ -734,6 +784,7 @@
 			break;
 		}
 		wdev_unlock(wdev);
+		rdev->opencount++;
 		mutex_unlock(&rdev->devlist_mtx);
 		cfg80211_unlock_rdev(rdev);
 #endif
@@ -756,7 +807,6 @@
 			sysfs_remove_link(&dev->dev.kobj, "phy80211");
 			list_del_init(&wdev->list);
 			rdev->devlist_generation++;
-			mutex_destroy(&wdev->mtx);
 #ifdef CONFIG_WIRELESS_EXT
 			kfree(wdev->wext.keys);
 #endif
@@ -50,6 +50,8 @@
 	struct mutex devlist_mtx;
 	struct list_head netdev_list;
 	int devlist_generation;
+	int opencount; /* also protected by devlist_mtx */
+	wait_queue_head_t dev_wait;
  
 	/* BSSes/scanning */
 	spinlock_t bss_lock;
...	...	@@ -1325,6 +1325,8 @@
1325	1325
1326	1326	struct mutex mtx;
1327	1327
	1328	+ struct work_struct cleanup_work;
	1329	+
1328	1330	/* currently used for IBSS and SME - might be rearranged later */
1329	1331	u8 ssid[IEEE80211_MAX_SSID_LEN];
1330	1332	u8 ssid_len;
...	...	@@ -430,6 +430,8 @@
430	430	INIT_WORK(&rdev->conn_work, cfg80211_conn_work);
431	431	INIT_WORK(&rdev->event_work, cfg80211_event_work);
432	432
	433	+ init_waitqueue_head(&rdev->dev_wait);
	434	+
433	435	/*
434	436	* Initialize wiphy parameters to IEEE 802.11 MIB default values.
435	437	* Fragmentation and RTS threshold are disabled by default with the
436	438
437	439
438	440
439	441
440	442
441	443
...	...	@@ -574,34 +576,42 @@
574	576	/* protect the device list */
575	577	mutex_lock(&cfg80211_mutex);
576	578
	579	+ wait_event(rdev->dev_wait, ({
	580	+ int __count;
	581	+ mutex_lock(&rdev->devlist_mtx);
	582	+ __count = rdev->opencount;
	583	+ mutex_unlock(&rdev->devlist_mtx);
	584	+ __count == 0;}));
	585	+
	586	+ mutex_lock(&rdev->devlist_mtx);
577	587	BUG_ON(!list_empty(&rdev->netdev_list));
	588	+ mutex_unlock(&rdev->devlist_mtx);
578	589
579	590	/*
	591	+ * First remove the hardware from everywhere, this makes
	592	+ * it impossible to find from userspace.
	593	+ */
	594	+ cfg80211_debugfs_rdev_del(rdev);
	595	+ list_del(&rdev->list);
	596	+
	597	+ /*
580	598	* Try to grab rdev->mtx. If a command is still in progress,
581	599	* hopefully the driver will refuse it since it's tearing
582	600	* down the device already. We wait for this command to complete
583	601	* before unlinking the item from the list.
584	602	* Note: as codified by the BUG_ON above we cannot get here if
585		- * a virtual interface is still associated. Hence, we can only
586		- * get to lock contention here if userspace issues a command
587		- * that identified the hardware by wiphy index.
	603	+ * a virtual interface is still present. Hence, we can only get
	604	+ * to lock contention here if userspace issues a command that
	605	+ * identified the hardware by wiphy index.
588	606	*/
589	607	cfg80211_lock_rdev(rdev);
590		-
591		- if (WARN_ON(rdev->scan_req)) {
592		- rdev->scan_req->aborted = true;
593		- ___cfg80211_scan_done(rdev);
594		- }
595		-
	608	+ /* nothing */
596	609	cfg80211_unlock_rdev(rdev);
597	610
598		- cfg80211_debugfs_rdev_del(rdev);
599		-
600	611	/* If this device got a regulatory hint tell core its
601	612	* free to listen now to a new shiny device regulatory hint */
602	613	reg_device_remove(wiphy);
603	614
604		- list_del(&rdev->list);
605	615	cfg80211_rdev_list_generation++;
606	616	device_del(&rdev->wiphy.dev);
607	617	debugfs_remove(rdev->wiphy.debugfsdir);
...	...	@@ -640,6 +650,31 @@
640	650	}
641	651	EXPORT_SYMBOL(wiphy_rfkill_set_hw_state);
642	652
	653	+static void wdev_cleanup_work(struct work_struct *work)
	654	+{
	655	+ struct wireless_dev *wdev;
	656	+ struct cfg80211_registered_device *rdev;
	657	+
	658	+ wdev = container_of(work, struct wireless_dev, cleanup_work);
	659	+ rdev = wiphy_to_dev(wdev->wiphy);
	660	+
	661	+ cfg80211_lock_rdev(rdev);
	662	+
	663	+ if (WARN_ON(rdev->scan_req && rdev->scan_req->dev == wdev->netdev)) {
	664	+ rdev->scan_req->aborted = true;
	665	+ ___cfg80211_scan_done(rdev);
	666	+ }
	667	+
	668	+ cfg80211_unlock_rdev(rdev);
	669	+
	670	+ mutex_lock(&rdev->devlist_mtx);
	671	+ rdev->opencount--;
	672	+ mutex_unlock(&rdev->devlist_mtx);
	673	+ wake_up(&rdev->dev_wait);
	674	+
	675	+ dev_put(wdev->netdev);
	676	+}
	677	+
643	678	static int cfg80211_netdev_notifier_call(struct notifier_block * nb,
644	679	unsigned long state,
645	680	void *ndev)
...	...	@@ -663,6 +698,7 @@
663	698	* are added with nl80211.
664	699	*/
665	700	mutex_init(&wdev->mtx);
	701	+ INIT_WORK(&wdev->cleanup_work, wdev_cleanup_work);
666	702	INIT_LIST_HEAD(&wdev->event_list);
667	703	spin_lock_init(&wdev->event_lock);
668	704	mutex_lock(&rdev->devlist_mtx);
669	705
...	...	@@ -717,8 +753,22 @@
717	753	default:
718	754	break;
719	755	}
	756	+ dev_hold(dev);
	757	+ schedule_work(&wdev->cleanup_work);
720	758	break;
721	759	case NETDEV_UP:
	760	+ /*
	761	+ * If we have a really quick DOWN/UP succession we may
	762	+ * have this work still pending ... cancel it and see
	763	+ * if it was pending, in which case we need to account
	764	+ * for some of the work it would have done.
	765	+ */
	766	+ if (cancel_work_sync(&wdev->cleanup_work)) {
	767	+ mutex_lock(&rdev->devlist_mtx);
	768	+ rdev->opencount--;
	769	+ mutex_unlock(&rdev->devlist_mtx);
	770	+ dev_put(dev);
	771	+ }
722	772	#ifdef CONFIG_WIRELESS_EXT
723	773	cfg80211_lock_rdev(rdev);
724	774	mutex_lock(&rdev->devlist_mtx);
...	...	@@ -734,6 +784,7 @@
734	784	break;
735	785	}
736	786	wdev_unlock(wdev);
	787	+ rdev->opencount++;
737	788	mutex_unlock(&rdev->devlist_mtx);
738	789	cfg80211_unlock_rdev(rdev);
739	790	#endif
...	...	@@ -756,7 +807,6 @@
756	807	sysfs_remove_link(&dev->dev.kobj, "phy80211");
757	808	list_del_init(&wdev->list);
758	809	rdev->devlist_generation++;
759		- mutex_destroy(&wdev->mtx);
760	810	#ifdef CONFIG_WIRELESS_EXT
761	811	kfree(wdev->wext.keys);
762	812	#endif
...	...	@@ -50,6 +50,8 @@
50	50	struct mutex devlist_mtx;
51	51	struct list_head netdev_list;
52	52	int devlist_generation;
	53	+ int opencount; /* also protected by devlist_mtx */
	54	+ wait_queue_head_t dev_wait;
53	55
54	56	/* BSSes/scanning */
55	57	spinlock_t bss_lock;