mac80211: fix scan race, simplify code

The scan code has a race that Michael reported
he ran into, but it's easy to fix while at the
same time simplifying the code.

The race resulted in the following warning:

------------[ cut here ]------------
WARNING: at net/mac80211/scan.c:310 ieee80211_rx_bss_free+0x20c/0x4b8 [mac80211]()
Modules linked in: [...]
[<c0033edc>] (unwind_backtrace+0x0/0xe0) from [<c004f2a4>] (warn_slowpath_common+0x4c/0x64)
[... backtrace wasn't useful ...]

Reported-by: Michael Buesch <mb@bu3sch.de>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 842954509925cbda1476da0155e5008421a564a5..489b6ad200d4b3933f5a9e5c3dd40d5a43b51bde 100644 (file)
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -258,10 +258,12 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_local *local)
        return true;
 }
 
-static bool __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
+static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
                                       bool was_hw_scan)
 {
        struct ieee80211_local *local = hw_to_local(hw);
+       bool on_oper_chan;
+       bool enable_beacons = false;
 
        lockdep_assert_held(&local->mtx);
 
@@ -275,12 +277,12 @@ static bool __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
                aborted = true;
 
        if (WARN_ON(!local->scan_req))
-               return false;
+               return;
 
        if (was_hw_scan && !aborted && ieee80211_prep_hw_scan(local)) {
                int rc = drv_hw_scan(local, local->scan_sdata, local->hw_scan_req);
                if (rc == 0)
-                       return false;
+                       return;
        }
 
        kfree(local->hw_scan_req);
@@ -294,26 +296,11 @@ static bool __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
        local->scanning = 0;
        local->scan_channel = NULL;
 
-       return true;
-}
-
-static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
-                                             bool was_hw_scan)
-{
-       struct ieee80211_local *local = hw_to_local(hw);
-       bool on_oper_chan;
-       bool enable_beacons = false;
-
-       mutex_lock(&local->mtx);
        on_oper_chan = ieee80211_cfg_on_oper_channel(local);
 
-       WARN_ON(local->scanning & (SCAN_SW_SCANNING | SCAN_HW_SCANNING));
-
-       if (was_hw_scan || !on_oper_chan) {
-               if (WARN_ON(local->scan_channel))
-                       local->scan_channel = NULL;
+       if (was_hw_scan || !on_oper_chan)
                ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
-       } else
+       else
                /* Set power back to normal operating levels. */
                ieee80211_hw_config(local, 0);
 
@@ -331,7 +318,6 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
        }
 
        ieee80211_recalc_idle(local);
-       mutex_unlock(&local->mtx);
 
        ieee80211_mlme_notify_scan_completed(local);
        ieee80211_ibss_notify_scan_completed(local);
@@ -686,12 +672,14 @@ void ieee80211_scan_work(struct work_struct *work)
 {
        struct ieee80211_local *local =
                container_of(work, struct ieee80211_local, scan_work.work);
-       struct ieee80211_sub_if_data *sdata = local->scan_sdata;
+       struct ieee80211_sub_if_data *sdata;
        unsigned long next_delay = 0;
-       bool aborted, hw_scan, finish;
+       bool aborted, hw_scan;
 
        mutex_lock(&local->mtx);
 
+       sdata = local->scan_sdata;
+
        if (test_and_clear_bit(SCAN_COMPLETED, &local->scanning)) {
                aborted = test_and_clear_bit(SCAN_ABORTED, &local->scanning);
                goto out_complete;
@@ -755,17 +743,11 @@ void ieee80211_scan_work(struct work_struct *work)
        } while (next_delay == 0);
 
        ieee80211_queue_delayed_work(&local->hw, &local->scan_work, next_delay);
-       mutex_unlock(&local->mtx);
-       return;
+       goto out;
 
 out_complete:
        hw_scan = test_bit(SCAN_HW_SCANNING, &local->scanning);
-       finish = __ieee80211_scan_completed(&local->hw, aborted, hw_scan);
-       mutex_unlock(&local->mtx);
-       if (finish)
-               __ieee80211_scan_completed_finish(&local->hw, hw_scan);
-       return;
-
+       __ieee80211_scan_completed(&local->hw, aborted, hw_scan);
 out:
        mutex_unlock(&local->mtx);
 }
@@ -835,7 +817,6 @@ int ieee80211_request_internal_scan(struct ieee80211_sub_if_data *sdata,
 void ieee80211_scan_cancel(struct ieee80211_local *local)
 {
        bool abortscan;
-       bool finish = false;
 
        /*
         * We are only canceling software scan, or deferred scan that was not
@@ -855,14 +836,17 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
 
        mutex_lock(&local->mtx);
        abortscan = local->scan_req && !test_bit(SCAN_HW_SCANNING, &local->scanning);
-       if (abortscan)
-               finish = __ieee80211_scan_completed(&local->hw, true, false);
-       mutex_unlock(&local->mtx);
-
        if (abortscan) {
-               /* The scan is canceled, but stop work from being pending */
-               cancel_delayed_work_sync(&local->scan_work);
+               /*
+                * The scan is canceled, but stop work from being pending.
+                *
+                * If the work is currently running, it must be blocked on
+                * the mutex, but we'll set scan_sdata = NULL and it'll
+                * simply exit once it acquires the mutex.
+                */
+               cancel_delayed_work(&local->scan_work);
+               /* and clean up */
+               __ieee80211_scan_completed(&local->hw, true, false);
        }
-       if (finish)
-               __ieee80211_scan_completed_finish(&local->hw, false);
+       mutex_unlock(&local->mtx);
 }