diff mbox series

[13/16] WNM: Consolidate the scanning paths for BTM requests

Message ID 20240429115157.211073-14-benjamin@sipsolutions.net
State Accepted
Headers show
Series BTM refactorings and abridged bit handling | expand

Commit Message

Benjamin Berg April 29, 2024, 11:51 a.m. UTC 
From: Benjamin Berg <benjamin.berg@intel.com>

There was an ancient code path to trigger a scan that was apparently
forgotten when the code was extended over time. It does not make any
sense to trigger a scan twice, so remove the earlier scan.

The earlier scan call was avoiding to trigger a new scan if a fixed
BSSID is configured. This seems like a reasonable restriction to do, so
add this check before starting a scan.

Consolidate everything so that scanning happens at the end of the
functions unless we bail out before. Add a "reset" label for all other
cases to ensure that we don't leave things in the a bad state.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
---
 tests/hwsim/test_ap_hs20.py   |   1 +
 tests/hwsim/test_hapd_ctrl.py |   1 +
 tests/hwsim/test_wnm.py       |   2 +
 wpa_supplicant/wnm_sta.c      | 129 +++++++++++++++++++---------------
 4 files changed, 75 insertions(+), 58 deletions(-)

Comments

Jouni Malinen Aug. 2, 2024, 10:26 a.m. UTC | #1 
On Mon, Apr 29, 2024 at 01:51:54PM +0200, benjamin@sipsolutions.net wrote:
> There was an ancient code path to trigger a scan that was apparently
> forgotten when the code was extended over time. It does not make any
> sense to trigger a scan twice, so remove the earlier scan.
> 
> The earlier scan call was avoiding to trigger a new scan if a fixed
> BSSID is configured. This seems like a reasonable restriction to do, so
> add this check before starting a scan.
> 
> Consolidate everything so that scanning happens at the end of the
> functions unless we bail out before. Add a "reset" label for all other
> cases to ensure that we don't leave things in the a bad state.

>  tests/hwsim/test_ap_hs20.py   |   1 +
>  tests/hwsim/test_hapd_ctrl.py |   1 +
>  tests/hwsim/test_wnm.py       |   2 +
>  wpa_supplicant/wnm_sta.c      | 129 +++++++++++++++++++---------------

Those test case changes do not seem to have anything to do with this
commit message, so I'm dropping them from this patch.

> diff --git a/tests/hwsim/test_ap_hs20.py b/tests/hwsim/test_ap_hs20.py
> @@ -2813,6 +2813,7 @@ def _test_ap_hs20_session_info(dev, apdev):
>      interworking_select(dev[0], bssid, freq="2412")
>      interworking_connect(dev[0], bssid, "TTLS")
> +    dev[0].flush_scan_cache()

..

It feels strange to flush the scan results after connection, so if these
are really needed, there needs to be a clear commit message that
describe the reasons.
Benjamin Berg Aug. 2, 2024, 10:32 a.m. UTC | #2 
Hi,

just a quick response. The reason the test change is needed is because the test verifies a scan is being done. However, the code has a fast path to not scan if there are recent results available.

Before the change, a scan happened anyway, but that isn't the case anymore  making the flush (or sufficiently long sleep) necessary.

But I agree, I should have pointed out in the commit message why the test changes are needes (and why one can argue that it is a fix of the test behavior).

Benjamin 

On Friday, 2 August 2024, Jouni Malinen wrote:
> On Mon, Apr 29, 2024 at 01:51:54PM +0200, benjamin@sipsolutions.net wrote:
> > There was an ancient code path to trigger a scan that was apparently
> > forgotten when the code was extended over time. It does not make any
> > sense to trigger a scan twice, so remove the earlier scan.
> > 
> > The earlier scan call was avoiding to trigger a new scan if a fixed
> > BSSID is configured. This seems like a reasonable restriction to do, so
> > add this check before starting a scan.
> > 
> > Consolidate everything so that scanning happens at the end of the
> > functions unless we bail out before. Add a "reset" label for all other
> > cases to ensure that we don't leave things in the a bad state.
> 
> >  tests/hwsim/test_ap_hs20.py   |   1 +
> >  tests/hwsim/test_hapd_ctrl.py |   1 +
> >  tests/hwsim/test_wnm.py       |   2 +
> >  wpa_supplicant/wnm_sta.c      | 129 +++++++++++++++++++---------------
> 
> Those test case changes do not seem to have anything to do with this
> commit message, so I'm dropping them from this patch.
> 
> > diff --git a/tests/hwsim/test_ap_hs20.py b/tests/hwsim/test_ap_hs20.py
> > @@ -2813,6 +2813,7 @@ def _test_ap_hs20_session_info(dev, apdev):
> >      interworking_select(dev[0], bssid, freq="2412")
> >      interworking_connect(dev[0], bssid, "TTLS")
> > +    dev[0].flush_scan_cache()
> 
> ..
> 
> It feels strange to flush the scan results after connection, so if these
> are really needed, there needs to be a clear commit message that
> describe the reasons.
> 
> -- 
> Jouni Malinen                                            PGP id EFC895FA
> 
>
diff mbox series

Patch

diff --git a/tests/hwsim/test_ap_hs20.py b/tests/hwsim/test_ap_hs20.py
index 9cf2d9040..bfdbee1c5 100644
--- a/tests/hwsim/test_ap_hs20.py
+++ b/tests/hwsim/test_ap_hs20.py
@@ -2813,6 +2813,7 @@  def _test_ap_hs20_session_info(dev, apdev):
                             'password': "password"})
     interworking_select(dev[0], bssid, freq="2412")
     interworking_connect(dev[0], bssid, "TTLS")
+    dev[0].flush_scan_cache()
     ev = dev[0].wait_event(["ESS-DISASSOC-IMMINENT"], timeout=10)
     if ev is None:
         raise Exception("Timeout on ESS disassociation imminent notice")
diff --git a/tests/hwsim/test_hapd_ctrl.py b/tests/hwsim/test_hapd_ctrl.py
index 9cf8ac73c..5b8361f9f 100644
--- a/tests/hwsim/test_hapd_ctrl.py
+++ b/tests/hwsim/test_hapd_ctrl.py
@@ -249,6 +249,7 @@  def test_hapd_ctrl_disassoc_imminent(dev, apdev):
         raise Exception("Unexpected DISASSOC_IMMINENT success")
     if "FAIL" not in hapd.request("DISASSOC_IMMINENT 00:11:22:33:44:55 2"):
         raise Exception("Unexpected DISASSOC_IMMINENT success")
+    dev[0].flush_scan_cache()
     dev[0].connect(ssid, key_mgmt="NONE", scan_freq="2412")
     addr = dev[0].p2p_interface_addr()
     if "OK" not in hapd.request("DISASSOC_IMMINENT " + addr + " 2"):
diff --git a/tests/hwsim/test_wnm.py b/tests/hwsim/test_wnm.py
index ecaf19009..4df9d77f3 100644
--- a/tests/hwsim/test_wnm.py
+++ b/tests/hwsim/test_wnm.py
@@ -120,6 +120,7 @@  def test_wnm_disassoc_imminent(dev, apdev):
     """WNM Disassociation Imminent"""
     hapd = start_wnm_ap(apdev[0], time_adv=True, wnm_sleep_mode=True)
     dev[0].connect("test-wnm", key_mgmt="NONE", scan_freq="2412")
+    dev[0].flush_scan_cache()
     addr = dev[0].p2p_interface_addr()
     hapd.request("DISASSOC_IMMINENT " + addr + " 10")
     ev = dev[0].wait_event(["WNM: Disassociation Imminent"])
@@ -137,6 +138,7 @@  def test_wnm_disassoc_imminent_bssid_set(dev, apdev):
     hapd2 = start_wnm_ap(apdev[1], time_adv=True, wnm_sleep_mode=True)
     dev[0].connect("test-wnm", key_mgmt="NONE", bssid=hapd.own_addr(),
                    scan_freq="2412")
+    dev[0].flush_scan_cache()
     addr = dev[0].own_addr()
     cmd = "BSS_TM_REQ " + addr + " pref=1 disassoc_imminent=1 disassoc_timer=100 neighbor=" + apdev[1]['bssid'] + ",0x0000," + "81,1,7,0301ff"
     if "OK" not in hapd.request(cmd):
diff --git a/wpa_supplicant/wnm_sta.c b/wpa_supplicant/wnm_sta.c
index f0cbf914c..f4986a3bc 100644
--- a/wpa_supplicant/wnm_sta.c
+++ b/wpa_supplicant/wnm_sta.c
@@ -1349,8 +1349,7 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 
 	if (!wpa_s->wnm_dialog_token) {
 		wpa_printf(MSG_DEBUG, "WNM: invalid dialog token");
-		wnm_btm_reset(wpa_s);
-		return;
+		goto reset;
 	}
 
 #if defined(CONFIG_MBO) && defined(CONFIG_TESTING_OPTIONS)
@@ -1361,7 +1360,7 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 		wnm_send_bss_transition_mgmt_resp(
 			wpa_s, wpa_s->reject_btm_req_reason,
 			MBO_TRANSITION_REJECT_REASON_UNSPECIFIED, 0, NULL);
-		return;
+		goto reset;
 	}
 #endif /* CONFIG_MBO && CONFIG_TESTING_OPTIONS */
 
@@ -1370,7 +1369,7 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 	if (wpa_s->wnm_mode & WNM_BSS_TM_REQ_BSS_TERMINATION_INCLUDED) {
 		if (end - pos < 12) {
 			wpa_printf(MSG_DEBUG, "WNM: Too short BSS TM Request");
-			return;
+			goto reset;
 		}
 		os_memcpy(wpa_s->wnm_bss_termination_duration, pos, 12);
 		pos += 12; /* BSS Termination Duration */
@@ -1383,13 +1382,13 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 		if (end - pos < 1) {
 			wpa_printf(MSG_DEBUG, "WNM: Invalid BSS Transition "
 				   "Management Request (URL)");
-			return;
+			goto reset;
 		}
 		url_len = *pos++;
 		if (url_len > end - pos) {
 			wpa_printf(MSG_DEBUG,
 				   "WNM: Invalid BSS Transition Management Request (URL truncated)");
-			return;
+			goto reset;
 		}
 		os_memcpy(url, pos, url_len);
 		url[url_len] = '\0';
@@ -1424,7 +1423,7 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 			wnm_send_bss_transition_mgmt_resp(
 				wpa_s, WNM_BSS_TM_ACCEPT, 0, 0, NULL);
 
-			return;
+			goto reset;
 		}
 
 		/* The last link is being removed (which must be the assoc link)
@@ -1443,16 +1442,9 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 		os_memcpy(wpa_s->wnm_disassoc_addr, wpa_s->bssid, ETH_ALEN);
 	}
 
-	if (disassoc_imminent) {
+	if (disassoc_imminent)
 		wpa_msg(wpa_s, MSG_INFO, "WNM: Disassociation Imminent - "
 			"Disassociation Timer %u", wpa_s->wnm_dissoc_timer);
-		if (wpa_s->wnm_dissoc_timer && !wpa_s->scanning &&
-		    (!wpa_s->current_ssid || !wpa_s->current_ssid->bssid_set)) {
-			wpa_printf(MSG_DEBUG, "Trying to find another BSS");
-			wpa_s->wnm_transition_scan = true;
-			wpa_supplicant_req_scan(wpa_s, 0, 0);
-		}
-	}
 
 #ifdef CONFIG_MBO
 	vendor = get_ie(pos, end - pos, WLAN_EID_VENDOR_SPECIFIC);
@@ -1466,7 +1458,7 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 		wpa_msg(wpa_s, MSG_INFO, "WNM: Preferred List Available");
 
 		if (wnm_parse_candidate_list(wpa_s, pos, end) < 0)
-			return;
+			goto reset;
 
 		if (!wpa_s->wnm_num_neighbor_report) {
 			wpa_printf(MSG_DEBUG,
@@ -1475,17 +1467,7 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 				wpa_s, WNM_BSS_TM_REJECT_NO_SUITABLE_CANDIDATES,
 				MBO_TRANSITION_REJECT_REASON_UNSPECIFIED, 0,
 				NULL);
-			return;
-		}
-
-		if (wpa_s->current_ssid && wpa_s->current_ssid->bssid_set) {
-			wpa_printf(MSG_DEBUG,
-				   "WNM: Configuration prevents roaming (BSSID set)");
-			wnm_send_bss_transition_mgmt_resp(
-				wpa_s, WNM_BSS_TM_REJECT_NO_SUITABLE_CANDIDATES,
-				MBO_TRANSITION_REJECT_REASON_UNSPECIFIED, 0,
-				NULL);
-			return;
+			goto reset;
 		}
 
 		wnm_sort_cand_list(wpa_s);
@@ -1495,36 +1477,11 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 			   valid_ms);
 		os_get_reltime(&wpa_s->wnm_cand_valid_until);
 		os_reltime_add_ms(&wpa_s->wnm_cand_valid_until, valid_ms);
-
-		/*
-		* Try fetching the latest scan results from the kernel.
-		* This can help in finding more up-to-date information should
-		* the driver have done some internal scanning operations after
-		* the last scan result update in wpa_supplicant.
-		*
-		* It is not a new scan, this does not update the last_scan
-		* timestamp nor will it expire old BSSs.
-		*/
-		wpa_supplicant_update_scan_results(wpa_s, NULL);
-		if (wnm_scan_process(wpa_s, true) > 0)
-			return;
-		wpa_printf(MSG_DEBUG,
-			   "WNM: No valid match in previous scan results - try a new scan");
-
-		wnm_set_scan_freqs(wpa_s);
-		if (wpa_s->wnm_num_neighbor_report == 1) {
-			os_memcpy(wpa_s->next_scan_bssid,
-				  wpa_s->wnm_neighbor_report_elements[0].bssid,
-				  ETH_ALEN);
-			wpa_printf(MSG_DEBUG,
-				   "WNM: Scan only for a specific BSSID since there is only a single candidate "
-				   MACSTR, MAC2STR(wpa_s->next_scan_bssid));
-		}
-		wpa_s->wnm_transition_scan = true;
-		wpa_supplicant_req_scan(wpa_s, 0, 0);
-	} else if (reply) {
+	} else if (!disassoc_imminent) {
 		enum bss_trans_mgmt_status_code status;
 
+		/* No candidate list and disassociation is not imminent */
+
 		if ((wpa_s->wnm_mode & WNM_BSS_TM_REQ_ESS_DISASSOC_IMMINENT) ||
 		    wpa_s->wnm_link_removal)
 			status = WNM_BSS_TM_ACCEPT;
@@ -1532,10 +1489,66 @@  static void ieee802_11_rx_bss_trans_mgmt_req(struct wpa_supplicant *wpa_s,
 			wpa_msg(wpa_s, MSG_INFO, "WNM: BSS Transition Management Request did not include candidates");
 			status = WNM_BSS_TM_REJECT_UNSPECIFIED;
 		}
-		wnm_send_bss_transition_mgmt_resp(
-			wpa_s, status,
-			MBO_TRANSITION_REJECT_REASON_UNSPECIFIED, 0, NULL);
+
+		if (reply)
+			wnm_send_bss_transition_mgmt_resp(
+				wpa_s, status,
+				MBO_TRANSITION_REJECT_REASON_UNSPECIFIED, 0,
+				NULL);
+
+		goto reset;
 	}
+
+	/*
+	 * Try fetching the latest scan results from the kernel.
+	 * This can help in finding more up-to-date information should
+	 * the driver have done some internal scanning operations after
+	 * the last scan result update in wpa_supplicant.
+	 *
+	 * It is not a new scan, this does not update the last_scan
+	 * timestamp nor will it expire old BSSs.
+	 */
+	wpa_supplicant_update_scan_results(wpa_s, NULL);
+	if (wnm_scan_process(wpa_s, true) > 0)
+		return;
+	wpa_printf(MSG_DEBUG,
+		   "WNM: No valid match in previous scan results - try a new scan");
+
+	/*
+	 * If we have a fixed BSSID configured, just reject at this point.
+	 * NOTE: We could actually check if we are allowed to stay (and we do
+	 * above if we have scan results available).
+	 */
+	if (wpa_s->current_ssid && wpa_s->current_ssid->bssid_set) {
+		wpa_printf(MSG_DEBUG, "WNM: Fixed BSSID, rejecting request");
+
+		if (reply)
+			wnm_send_bss_transition_mgmt_resp(
+				wpa_s,
+				WNM_BSS_TM_REJECT_NO_SUITABLE_CANDIDATES,
+				0, 0, NULL);
+
+		goto reset;
+	}
+
+	wnm_set_scan_freqs(wpa_s);
+	if (wpa_s->wnm_num_neighbor_report == 1) {
+		os_memcpy(wpa_s->next_scan_bssid,
+			  wpa_s->wnm_neighbor_report_elements[0].bssid,
+			  ETH_ALEN);
+		wpa_printf(MSG_DEBUG,
+			  "WNM: Scan only for a specific BSSID since there is only a single candidate "
+			  MACSTR, MAC2STR(wpa_s->next_scan_bssid));
+	}
+	wpa_s->wnm_transition_scan = true;
+	wpa_supplicant_req_scan(wpa_s, 0, 0);
+
+	/* Continue from scan handler */
+	return;
+
+reset:
+	wnm_btm_reset(wpa_s);
+	return;
 }