diff mbox series

[08/16] WNM: Move driver MBO transition rejection into wnm_is_bss_excluded

Message ID 20240429115157.211073-9-benjamin@sipsolutions.net
State Changes Requested
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>

Change the logic a bit to not directly use the result of the
wpa_drv_get_bss_trans_status call and instead use the same selection
logic as usual but taking into account the driver rejections.

This changes the logic in minor ways. The main change is that this
aligns the ordering of BSSs to be identical in all cases. More
precisely, we'll select the best BSS as found by find_better_target.

Beyond that, it also means that in the case of an non-abridged BTM
request we'll also consider candidates that were found through the scan
and not in the neighbor report. In this case, the driver will not have a
chance to reject them.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
---
 wpa_supplicant/wnm_sta.c | 138 +++++++++++++++++++--------------------
 wpa_supplicant/wnm_sta.h |   2 +-
 2 files changed, 70 insertions(+), 70 deletions(-)

Comments

Jouni Malinen Aug. 2, 2024, 10:30 a.m. UTC | #1 
On Mon, Apr 29, 2024 at 01:51:49PM +0200, benjamin@sipsolutions.net wrote:
> Change the logic a bit to not directly use the result of the
> wpa_drv_get_bss_trans_status call and instead use the same selection
> logic as usual but taking into account the driver rejections.
> 
> This changes the logic in minor ways. The main change is that this
> aligns the ordering of BSSs to be identical in all cases. More
> precisely, we'll select the best BSS as found by find_better_target.
> 
> Beyond that, it also means that in the case of an non-abridged BTM
> request we'll also consider candidates that were found through the scan
> and not in the neighbor report. In this case, the driver will not have a
> chance to reject them.

This feels a bit scary especially if the updated design has not actually
been tested with a driver that uses the driver-based mechanism for
updating candidate lists (which I'm assuming has not been done here).

And there is something strange here:

> diff --git a/wpa_supplicant/wnm_sta.c b/wpa_supplicant/wnm_sta.c
>  #ifdef CONFIG_MBO
> -static struct wpa_bss *
> -get_mbo_transition_candidate(struct wpa_supplicant *wpa_s,
> +static void
> +fetch_drv_mbo_candidate_info(struct wpa_supplicant *wpa_s,
>  			     enum mbo_transition_reject_reason *reason)
>  {

> +		if (nei->preference_present && nei->preference == 0)
>  			continue;
> +
> +		/* FIXME: Should we go through wpa_scan_res_match? */

Well, that is not the strange part, but again, something that makes me
unhappy about applying this without full testing.

> +#else
> +static void
> +fetch_drv_mbo_transition_candidate_info(struct wpa_supplicant *wpa_s)
> +{
>  }
>  #endif /* CONFIG_MBO */

That is the strange part.. What was that supposed to be? That same
function with empty payload? Now the function name is different and so
is the list of arguments. This would obviously not work without
CONFIG_MBO defined.

If this is just to skip the functionality within the function, defining
the function once with the actually body within #ifdef CONFIG_MBO would
be much cleaner.
Benjamin Berg Aug. 5, 2024, 5:41 p.m. UTC | #2 
Hi,

looks like I initially only replied in private by accident.

On Fri, 2024-08-02 at 13:30 +0300, Jouni Malinen wrote:
> On Mon, Apr 29, 2024 at 01:51:49PM +0200, benjamin@sipsolutions.net wrote:
> > Change the logic a bit to not directly use the result of the
> > wpa_drv_get_bss_trans_status call and instead use the same selection
> > logic as usual but taking into account the driver rejections.
> > 
> > This changes the logic in minor ways. The main change is that this
> > aligns the ordering of BSSs to be identical in all cases. More
> > precisely, we'll select the best BSS as found by find_better_target.
> > 
> > Beyond that, it also means that in the case of an non-abridged BTM
> > request we'll also consider candidates that were found through the scan
> > and not in the neighbor report. In this case, the driver will not have a
> > chance to reject them.
> 
> This feels a bit scary especially if the updated design has not actually
> been tested with a driver that uses the driver-based mechanism for
> updating candidate lists (which I'm assuming has not been done here).

Yes, it is a bit scary. Honestly, I am not able to test the code. Maybe
I missed it, but I didn't even find a driver that provides the nl80211
API.

> And there is something strange here:
> 
> > diff --git a/wpa_supplicant/wnm_sta.c b/wpa_supplicant/wnm_sta.c
> >  #ifdef CONFIG_MBO
> > -static struct wpa_bss *
> > -get_mbo_transition_candidate(struct wpa_supplicant *wpa_s,
> > +static void
> > +fetch_drv_mbo_candidate_info(struct wpa_supplicant *wpa_s,
> >  			     enum mbo_transition_reject_reason *reason)
> >  {
> 
> > +		if (nei->preference_present && nei->preference == 0)
> >  			continue;
> > +
> > +		/* FIXME: Should we go through wpa_scan_res_match? */
> 
> Well, that is not the strange part, but again, something that makes me
> unhappy about applying this without full testing.
> 
> > +#else
> > +static void
> > +fetch_drv_mbo_transition_candidate_info(struct wpa_supplicant *wpa_s)
> > +{
> >  }
> >  #endif /* CONFIG_MBO */
> 
> That is the strange part.. What was that supposed to be? That same
> function with empty payload? Now the function name is different and so
> is the list of arguments. This would obviously not work without
> CONFIG_MBO defined.

Oops, yep, looks like I messed up the function rename and such.

> If this is just to skip the functionality within the function, defining
> the function once with the actually body within #ifdef CONFIG_MBO would
> be much cleaner.

Sure, that makes sense!

Benjamin
diff mbox series

Patch

diff --git a/wpa_supplicant/wnm_sta.c b/wpa_supplicant/wnm_sta.c
index e8776a14b..64124348f 100644
--- a/wpa_supplicant/wnm_sta.c
+++ b/wpa_supplicant/wnm_sta.c
@@ -615,38 +615,37 @@  static void wnm_parse_neighbor_report(struct wpa_supplicant *wpa_s,
 }
 
 
-static void wnm_clear_acceptable(struct wpa_supplicant *wpa_s)
-{
-	unsigned int i;
-
-	for (i = 0; i < wpa_s->wnm_num_neighbor_report; i++)
-		wpa_s->wnm_neighbor_report_elements[i].acceptable = 0;
-}
-
 #ifdef CONFIG_MBO
-static struct wpa_bss *
-get_mbo_transition_candidate(struct wpa_supplicant *wpa_s,
+static void
+fetch_drv_mbo_candidate_info(struct wpa_supplicant *wpa_s,
 			     enum mbo_transition_reject_reason *reason)
 {
-	struct wpa_bss *target = NULL;
 	struct wpa_bss_trans_info params;
 	struct wpa_bss_candidate_info *info = NULL;
-	struct neighbor_report *nei = wpa_s->wnm_neighbor_report_elements;
-	u8 *first_candidate_bssid = NULL, *pos;
+	struct neighbor_report *nei;
+	u8 *pos;
 	unsigned int i;
 
+	if (!wpa_s->wnm_mbo_trans_reason_present)
+		return;
+
 	params.mbo_transition_reason = wpa_s->wnm_mbo_transition_reason;
 	params.n_candidates = 0;
 	params.bssid = os_calloc(wpa_s->wnm_num_neighbor_report, ETH_ALEN);
 	if (!params.bssid)
-		return NULL;
+		return;
 
 	pos = params.bssid;
-	for (i = 0; i < wpa_s->wnm_num_neighbor_report; nei++, i++) {
-		if (nei->is_first)
-			first_candidate_bssid = nei->bssid;
-		if (!nei->acceptable)
+	for (i = 0; i < wpa_s->wnm_num_neighbor_report; i++) {
+		nei = &wpa_s->wnm_neighbor_report_elements[i];
+
+		nei->drv_mbo_reject = 0;
+
+		if (nei->preference_present && nei->preference == 0)
 			continue;
+
+		/* FIXME: Should we go through wpa_scan_res_match? */
+
 		os_memcpy(pos, nei->bssid, ETH_ALEN);
 		pos += ETH_ALEN;
 		params.n_candidates++;
@@ -656,62 +655,40 @@  get_mbo_transition_candidate(struct wpa_supplicant *wpa_s,
 		goto end;
 
 	info = wpa_drv_get_bss_trans_status(wpa_s, &params);
-	if (!info) {
-		/* If failed to get candidate BSS transition status from driver,
-		 * get the first acceptable candidate from wpa_supplicant.
-		 */
-		target = wpa_bss_get_bssid(wpa_s, params.bssid);
+	if (!info)
 		goto end;
-	}
 
-	/* Get the first acceptable candidate from driver */
 	for (i = 0; i < info->num; i++) {
-		if (info->candidates[i].is_accept) {
-			target = wpa_bss_get_bssid(wpa_s,
-						   info->candidates[i].bssid);
-			goto end;
-		}
-	}
+		int j;
 
-	/* If Disassociation Imminent is set and driver rejects all the
-	 * candidate select first acceptable candidate which has
-	 * rssi > disassoc_imminent_rssi_threshold
-	 */
-	if (wpa_s->wnm_mode & WNM_BSS_TM_REQ_DISASSOC_IMMINENT) {
-		for (i = 0; i < info->num; i++) {
-			target = wpa_bss_get_bssid(wpa_s,
-						   info->candidates[i].bssid);
-			if (target &&
-			    (target->level <
-			     wpa_s->conf->disassoc_imminent_rssi_threshold))
+		for (j = 0; j < wpa_s->wnm_num_neighbor_report; nei++, j++) {
+			nei = &wpa_s->wnm_neighbor_report_elements[j];
+
+			if (!ether_addr_equal(info->candidates[i].bssid,
+					      nei->bssid))
 				continue;
-			goto end;
-		}
-	}
 
-	/* While sending BTM reject use reason code of the first candidate
-	 * received in BTM request frame
-	 */
-	if (reason) {
-		for (i = 0; i < info->num; i++) {
-			if (first_candidate_bssid &&
-			    ether_addr_equal(first_candidate_bssid,
-					     info->candidates[i].bssid)) {
+			nei->drv_mbo_reject = !info->candidates[i].is_accept;
+
+			/* Use the reject reason from "first" candidate */
+			if (nei->is_first && nei->drv_mbo_reject)
 				*reason = info->candidates[i].reject_reason;
-				break;
-			}
+
+			break;
 		}
 	}
 
-	target = NULL;
-
 end:
 	os_free(params.bssid);
 	if (info) {
 		os_free(info->candidates);
 		os_free(info);
 	}
-	return target;
+}
+#else
+static void
+fetch_drv_mbo_transition_candidate_info(struct wpa_supplicant *wpa_s)
+{
 }
 #endif /* CONFIG_MBO */
 
@@ -755,8 +732,6 @@  compare_scan_neighbor_results(struct wpa_supplicant *wpa_s,
 	wpa_printf(MSG_DEBUG, "WNM: Current BSS " MACSTR " RSSI %d",
 		   MAC2STR(wpa_s->bssid), bss->level);
 
-	wnm_clear_acceptable(wpa_s);
-
 	for (i = 0; i < wpa_s->wnm_num_neighbor_report; i++) {
 		struct neighbor_report *nei;
 
@@ -796,21 +771,12 @@  compare_scan_neighbor_results(struct wpa_supplicant *wpa_s,
 			continue;
 		}
 
-		nei->acceptable = 1;
-
 		best_target = find_better_target(target, best_target);
 		if (target == bss)
 			bss_in_list = bss;
 	}
 
-#ifdef CONFIG_MBO
-	if (wpa_s->wnm_mbo_trans_reason_present)
-		target = get_mbo_transition_candidate(wpa_s, reason);
-	else
-		target = best_target;
-#else /* CONFIG_MBO */
 	target = best_target;
-#endif /* CONFIG_MBO */
 
 	if (!target)
 		return NULL;
@@ -1179,8 +1145,39 @@  int wnm_scan_process(struct wpa_supplicant *wpa_s, bool pre_scan_check)
 
 	wpa_s->wnm_transition_scan = false;
 
+	/* Fetch MBO transition candidate rejection information from driver */
+	fetch_drv_mbo_candidate_info(wpa_s, &reason);
+
 	/* Compare the Neighbor Report and scan results */
 	bss = compare_scan_neighbor_results(wpa_s, &reason);
+#ifdef CONFIG_MBO
+	if (!bss && wpa_s->wnm_mbo_trans_reason_present &&
+	    wpa_s->wnm_mode & WNM_BSS_TM_REQ_DISASSOC_IMMINENT) {
+		int i;
+
+		/*
+		 * We didn't find any candidate, the driver had a say about
+		 * which targets to reject and disassociation is immiment.
+		 *
+		 * We should still try to roam, so retry after ignoring the
+		 * driver reject for any BSS that has an RSSI better than
+		 * disassoc_imminent_rssi_threshold.
+		 */
+		for (i = 0; i < wpa_s->wnm_num_neighbor_report; i++) {
+			struct neighbor_report *nei;
+
+			nei = &wpa_s->wnm_neighbor_report_elements[i];
+
+			bss = wpa_bss_get_bssid(wpa_s, nei->bssid);
+
+			if (bss->level >
+			    wpa_s->conf->disassoc_imminent_rssi_threshold)
+				nei->drv_mbo_reject = 0;
+		}
+
+		bss = compare_scan_neighbor_results(wpa_s, &reason);
+	}
+#endif /* CONFIG_MBO */
 
 	/*
 	 * If this is a pre-scan check, returning 0 will trigger a scan and
@@ -2079,6 +2076,9 @@  bool wnm_is_bss_excluded(struct wpa_supplicant *wpa_s, struct wpa_bss *bss)
 		if (nei->preference_present && nei->preference == 0)
 			return true;
 
+		if (nei->drv_mbo_reject)
+			return true;
+
 		break;
 	}
 
diff --git a/wpa_supplicant/wnm_sta.h b/wpa_supplicant/wnm_sta.h
index 235a838fa..5994027a4 100644
--- a/wpa_supplicant/wnm_sta.h
+++ b/wpa_supplicant/wnm_sta.h
@@ -44,9 +44,9 @@  struct neighbor_report {
 	unsigned int rm_capab_present:1;
 	unsigned int bearing_present:1;
 	unsigned int bss_term_present:1;
-	unsigned int acceptable:1;
 #ifdef CONFIG_MBO
 	unsigned int is_first:1;
+	unsigned int drv_mbo_reject:1;
 #endif /* CONFIG_MBO */
 	struct measurement_pilot *meas_pilot;
 	struct multiple_bssid *mul_bssid;