iwlwifi: rewrite iwl-scan.c to avoid race conditions

In this patch I'm trying to avoid hardware scanning race conditions that may lead to not call ieee80211_scan_completed() (what in consequences gives "WARNING: at net/wireless/core.c:614 wdev_cleanup_work+0xb7/0xf0"), or call iee80211_scan_completed() more then once (what gives " WARNING: at net/mac80211/scan.c:312 ieee80211_scan_completed+0x5f/0x1f1").

First problem (warning in wdev_cleanup_work) make any further scan request from cfg80211 are ignored by mac80211 with EBUSY error, hence Networkmanager can not perform successful scan and not allow to establish new connection. So after suspend/resume (but maybe not only then) user is not able to connect to wireless network again.

We can not relay on that the commands (start and abort scan) are successful. Even if they are successfully send to the hardware, we can not get back notification from firmware (i.e. firmware hung or it was reseted), or we can get notification when we actually perform abort scan in driver code or after that.

To assure we call ieee80211_scan_completed() only once when scan was started we use SCAN_SCANNING bit. Code path, which first clear STATUS_SCANNING bit will call ieee80211_scan_completed().
We do this in many cases, in scan complete notification, scan abort and timeout, firmware reset, etc. each time we check SCANNING bit.

Note: there are still some race conditions. I commented them in code, I'm still working on that problems.

drivers/net/wireless/iwlwifi/iwl-3945.h +1 -1

index bb2aeeb..98509c5 100644
@@ -295,7 +295,7 @@ extern const struct iwl_channel_info *iwl3945_get_channel_info(
 extern int iwl3945_rs_next_rate(struct iwl_priv *priv, int rate);
 
 /* scanning */
-void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
+int iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif);
 
 /* Requires full declaration of iwl_priv before including */
 #include "iwl-io.h"

drivers/net/wireless/iwlwifi/iwl-agn-lib.c +1 -35

index eedd71f..29a5e8f 100644
@@ -1147,7 +1147,7 @@ static int iwl_get_channels_for_scan(struct iwl_priv *priv,
 	return added;
 }
 
-void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
+int iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
 {
 	struct iwl_host_cmd cmd = {
 		.id = REPLY_SCAN_CMD,

@@ -1155,7 +1155,6 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
 		.flags = CMD_SIZE_HUGE,
 	};
 	struct iwl_scan_cmd *scan;
-	struct ieee80211_conf *conf = NULL;
 	u32 rate_flags = 0;
 	u16 cmd_len;
 	u16 rx_chain = 0;

@@ -1167,48 +1166,7 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
 	int  chan_mod;
 	u8 active_chains;
 	u8 scan_tx_antennas = priv->hw_params.valid_tx_ant;
-
-	conf = ieee80211_get_hw_conf(priv->hw);
-
-	cancel_delayed_work(&priv->scan_check);
-
-	if (!iwl_is_ready(priv)) {
-		IWL_WARN(priv, "request scan called when driver not ready.\n");
-		goto done;
-	}
-
-	/* Make sure the scan wasn't canceled before this queued work
-	 * was given the chance to run... */
-	if (!test_bit(STATUS_SCANNING, &priv->status))
-		goto done;
-
-	/* This should never be called or scheduled if there is currently
-	 * a scan active in the hardware. */
-	if (test_bit(STATUS_SCAN_HW, &priv->status)) {
-		IWL_DEBUG_INFO(priv, "Multiple concurrent scan requests in parallel. "
-			       "Ignoring second request.\n");
-		goto done;
-	}
-
-	if (test_bit(STATUS_EXIT_PENDING, &priv->status)) {
-		IWL_DEBUG_SCAN(priv, "Aborting scan due to device shutdown\n");
-		goto done;
-	}
-
-	if (test_bit(STATUS_SCAN_ABORTING, &priv->status)) {
-		IWL_DEBUG_HC(priv, "Scan request while abort pending.  Queuing.\n");
-		goto done;
-	}
-

drivers/net/wireless/iwlwifi/iwl-agn-lib.c +0 -0

 int iwlagn_manage_ibss_station(struct iwl_priv *priv,

drivers/net/wireless/iwlwifi/iwl-agn.c +2 -0

index 26bc048..7602765 100644
@@ -3075,13 +3075,27 @@ static void iwl_bg_restart(struct work_struct *data)
 		return;
 
 	if (test_and_clear_bit(STATUS_FW_ERROR, &priv->status)) {
+		bool scan_pending = false;
+
 		mutex_lock(&priv->mutex);
 		priv->vif = NULL;
 		priv->is_open = 0;

drivers/net/wireless/iwlwifi/iwl-agn.c +8 -0

+static int iwl_do_scan_abort(struct iwl_priv *priv)
 
+	int ret = 0;
 
+	if (test_bit(STATUS_SCANNING, &priv->status)) {
+		if (test_and_set_bit(STATUS_SCAN_ABORTING, &priv->status))
+			IWL_DEBUG_SCAN(priv, "Scan abort in progress.\n");
+		else {
+			ret = iwl_send_scan_abort(priv);
+			if (ret) {

+ * NOTE: priv->mutex must be held before calling this function

drivers/net/wireless/iwlwifi/iwl-agn.c +1 -2

+int iwl_scan_cancel_timeout(struct iwl_priv *priv, unsigned long ms)
 {
-	int len = 0;
-	u8 *pos = NULL;

drivers/net/wireless/iwlwifi/iwl-sta.c +0 -0

@@ -989,11 +989,11 @@ void iwl_update_tkip_key(struct iwl_priv *priv,
 	unsigned long flags;

Hi Stanislaw,

Sorry for the long delay here.

Quoted 14 lines

drivers/net/wireless/iwlwifi/iwl-agn.c +9 -0

 	if (test_and_clear_bit(STATUS_FW_ERROR, &priv->status)) {
+		bool scan_pending = false;
+
 		mutex_lock(&priv->mutex);
 		priv->vif = NULL;
 		priv->is_open = 0;
+		if (test_bit(STATUS_SCANNING, &priv->status) &&
+		    !priv->is_internal_short_scan) {
+			scan_pending = true;
+			clear_bit(STATUS_SCAN_HW, &priv->status);
+			clear_bit(STATUS_SCAN_ABORTING, &priv->status);
+		}
 		mutex_unlock(&priv->mutex);
+

Review

Quoted 2 lines

(quoted diff) +2 -0

+		if (scan_pending)
+			ieee80211_scan_completed(priv->hw, true);

Since we've had locking problems in such situations a lot, I'm just going to allow mac80211 to call scan_completed() from any context. That'll get rid of all the problems with the mutx here, so that you can move this code into a helper function (based on your description, I suspect I'll see it again in the patch)

Quoted 11 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +11 -0

+	if (test_bit(STATUS_SCANNING, &priv->status)) {
+		if (test_and_set_bit(STATUS_SCAN_ABORTING, &priv->status))
+			IWL_DEBUG_SCAN(priv, "Scan abort in progress.\n");
+		else {
+			ret = iwl_send_scan_abort(priv);
+			if (ret) {
+				clear_bit(STATUS_SCANNING, &priv->status);
+				clear_bit(STATUS_SCAN_HW, &priv->status);
+				clear_bit(STATUS_SCAN_ABORTING, &priv->status);
+
+				/* If we did internal scan and mac80211 does

drivers/net/wireless/iwlwifi/iwl-scan.c void iwl_internal_short_hw_scan(struct iwl_priv *priv) · line 391

Quoted 5 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +5 -0

+				 * not schedule scan by itself we do not
+				 * have to complete it */
+				if (priv->is_internal_short_scan &&
+				    priv->scan_request == NULL)
+					ret = 0;

Is that && really correct? It's just an extra check, right? I mean, scan_request is always NULL for internal short scans...

Also, if I make the change I just talked about earlier, could this function call ieee80211_scan_completed() itself?

drivers/net/wireless/iwlwifi/iwl-scan.c void iwl_internal_short_hw_scan(struct iwl_priv *priv) · line 423

Quoted 9 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +3 -5

+	unsigned long now = jiffies;
 
-	/* Make sure there is enough space for the probe request,
-	 * two mandatory IEs and the data */
-	left -= 24;
-	if (left < 0)
-		return 0;
+	ret = iwl_do_scan_abort(priv);
+	mutex_unlock(&priv->mutex);

I'd prefer if we never dropped the mutex in a function that requires being called with it held. That can break locking really badly in the caller without anybody noticing.

drivers/net/wireless/iwlwifi/iwl-scan.c void iwl_internal_short_hw_scan(struct iwl_priv *priv) · line 428

Quoted 5 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +4 -0

+	if (ret) {
+		ieee80211_scan_completed(priv->hw, true);
+		goto out;
+	}

I guess it's just because of this though, so that should go away.

drivers/net/wireless/iwlwifi/iwl-scan.c void iwl_internal_short_hw_scan(struct iwl_priv *priv) · line 434

Quoted 7 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +5 -1

-	len += 24;
+	while (time_before(jiffies, now + msecs_to_jiffies(ms))) {
+		if (!test_bit(STATUS_SCANNING, &priv->status))
+			break;
+		msleep(20);
+	}

Or because of this?

drivers/net/wireless/iwlwifi/iwl-scan.c void iwl_internal_short_hw_scan(struct iwl_priv *priv) · line 438

Quoted 6 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +3 -2

-	/* ...next IE... */
-	pos = &frame->u.probe_req.variable[0];
+	/* XXX: race condtion: we can sucessufly cancel scan, but
+	 *	a new scan request could arrive, so we can still
+	 *	have scanning pending */ 

Can a new request really arrive? mac80211 needs to be processing the completed first? Or is this maybe for internal short scans?

Hmm, an only tangentially related question: do we really need to do all these atomic bit operations? We hold the mutex everywhere anyway, no?

drivers/net/wireless/iwlwifi/iwl-sta.c void iwl_update_tkip_key(struct iwl_priv *priv, · line 996

Quoted 5 lines

drivers/net/wireless/iwlwifi/iwl-sta.c +3 -0

+	if (test_bit(STATUS_SCAN_HW, &priv->status)) {
+		/* just live w/ bad key and rely briefly on SW decryption */
 		return;
 	}
+	/* XXX: race condition: nothing prevent to start HW scanning now */

TBH, I don't even understand why we need to cancel the scan here. We're just updating a key ... and we don't really do that while scanning anyway since it's triggered only by receiving frames successfully...

Quoted 5 lines

Since we've had locking problems in such situations a lot, I'm just going to allow mac80211 to call scan_completed() from any context. That'll get rid of all the problems with the mutx here, so that you can move this code into a helper function (based on your description, I suspect I'll see it again in the patch)

This is mutex recursion problem, scan_completed() call ieee80211_hw_config() -> iwl_mac_config() -> mutex_lock(&priv->mutex). I should put short comment about that. Even on possibly removal of ieee80211_hw_config (for example when aborted==true) we still can have possible deadlock, because of mac80211 local->mtx and iwlwifi priv->mutex locking ordering.

Quoted 6 lines

drivers/net/wireless/iwlwifi/iwl-scan.c void iwl_internal_short_hw_scan(struct iwl_priv *priv) · line 391

Quoted 3 lines

drivers/net/wireless/iwlwifi/iwl-scan.c +3 -0

+				if (priv->is_internal_short_scan &&
+				    priv->scan_request == NULL)
+					ret = 0;

Is that && really correct? It's just an extra check, right? I mean, scan_request is always NULL for internal short scans...

Nope, your commit f84b29ec0a1ab767679d3f2428877b65f94bc3ff changed that :-)

Quoted 2 lines

Hmm, an only tangentially related question: do we really need to do all these atomic bit operations? We hold the mutex everywhere anyway, no?

No. This my way to write things in one line instead of two, but perhaps two lines version should be used to not confuse readers.

Quoted 9 lines

drivers/net/wireless/iwlwifi/iwl-sta.c void iwl_update_tkip_key(struct iwl_priv *priv, · line 996

Quoted 5 lines

drivers/net/wireless/iwlwifi/iwl-sta.c +3 -0

+	if (test_bit(STATUS_SCAN_HW, &priv->status)) {
+		/* just live w/ bad key and rely briefly on SW decryption */
 		return;
 	}
+	/* XXX: race condition: nothing prevent to start HW scanning now */

TBH, I don't even understand why we need to cancel the scan here. We're just updating a key ... and we don't really do that while scanning anyway since it's triggered only by receiving frames successfully...

I don't understand this also.