[PATCH 1/8] blacklist: include a blacklist reason when adding/finding

[PATCH 1/8] blacklist: include a blacklist reason when adding/finding

[James Prestwood]

To both prepare for some new blacklisting behavior and allow for
easier consolidation of the network-specific blacklist include a
reason enum for each entry. This allows IWD to differentiate
between multiple blacklist types. For now only the existing
"permanent" type is being added which prevents connections to that
BSS via autoconnect until it expires.

By including a type into each entry we now have additional search
criteria and can have multiple entires of the same BSS with
different reasons. This was done versus a bitmask because each
blacklist reason may have a different expiration time. We want to
maintain individual expirations to have the best "memory" of past
events rather than overwriting them.

Future patches will lump in the temporary network blacklist as well
as a new roaming blacklist type.
---
 src/blacklist.c | 81 +++++++++++++++++++++++++++++++++++++------------
 src/blacklist.h | 14 +++++++--
 src/network.c   |  3 +-
 src/station.c   | 12 +++++---
 4 files changed, 83 insertions(+), 27 deletions(-)

diff --git a/src/blacklist.c b/src/blacklist.c
index 21f85a75..b6583fdf 100644
--- a/src/blacklist.c
+++ b/src/blacklist.c
@@ -51,10 +51,46 @@ struct blacklist_entry {
 	uint8_t addr[6];
 	uint64_t added_time;
 	uint64_t expire_time;
+	enum blacklist_reason reason;
+};
+
+struct blacklist_search {
+	const uint8_t *addr;
+	enum blacklist_reason reason;
 };
 
 static struct l_queue *blacklist;
 
+static struct blacklist_entry *blacklist_entry_new(const uint8_t *addr,
+						enum blacklist_reason reason)
+{
+	struct blacklist_entry *entry;
+	uint64_t added;
+	uint64_t expires;
+
+	switch (reason) {
+	case BLACKLIST_REASON_PERMANENT:
+		if (!blacklist_initial_timeout)
+			return NULL;
+
+		added = l_time_now();
+		expires = l_time_offset(added, blacklist_initial_timeout);
+		break;
+	default:
+		l_warn("Unhandled blacklist reason: %u", reason);
+		return NULL;
+	}
+
+	entry = l_new(struct blacklist_entry, 1);
+
+	entry->added_time = added;
+	entry->expire_time = expires;
+	entry->reason = reason;
+	memcpy(entry->addr, addr, 6);
+
+	return entry;
+}
+
 static bool check_if_expired(void *data, void *user_data)
 {
 	struct blacklist_entry *entry = data;
@@ -79,24 +115,28 @@ static void blacklist_prune(void)
 static bool match_addr(const void *a, const void *b)
 {
 	const struct blacklist_entry *entry = a;
-	const uint8_t *addr = b;
+	const struct blacklist_search *search = b;
+
+	if (entry->reason != search->reason)
+		return false;
 
-	if (!memcmp(entry->addr, addr, 6))
+	if (!memcmp(entry->addr, search->addr, 6))
 		return true;
 
 	return false;
 }
 
-void blacklist_add_bss(const uint8_t *addr)
+void blacklist_add_bss(const uint8_t *addr, enum blacklist_reason reason)
 {
 	struct blacklist_entry *entry;
-
-	if (!blacklist_initial_timeout)
-		return;
+	struct blacklist_search search = {
+		.addr = addr,
+		.reason = reason
+	};
 
 	blacklist_prune();
 
-	entry = l_queue_find(blacklist, match_addr, addr);
+	entry = l_queue_find(blacklist, match_addr, &search);
 
 	if (entry) {
 		uint64_t offset = l_time_diff(entry->added_time,
@@ -112,25 +152,24 @@ void blacklist_add_bss(const uint8_t *addr)
 		return;
 	}
 
-	entry = l_new(struct blacklist_entry, 1);
-
-	entry->added_time = l_time_now();
-	entry->expire_time = l_time_offset(entry->added_time,
-						blacklist_initial_timeout);
-	memcpy(entry->addr, addr, 6);
-
-	l_queue_push_tail(blacklist, entry);
+	entry = blacklist_entry_new(addr, reason);
+	if (entry)
+		l_queue_push_tail(blacklist, entry);
 }
 
-bool blacklist_contains_bss(const uint8_t *addr)
+bool blacklist_contains_bss(const uint8_t *addr, enum blacklist_reason reason)
 {
 	bool ret;
 	uint64_t time_now;
 	struct blacklist_entry *entry;
+	struct blacklist_search search = {
+		.addr = addr,
+		.reason = reason
+	};
 
 	blacklist_prune();
 
-	entry = l_queue_find(blacklist, match_addr, addr);
+	entry = l_queue_find(blacklist, match_addr, &search);
 
 	if (!entry)
 		return false;
@@ -142,13 +181,17 @@ bool blacklist_contains_bss(const uint8_t *addr)
 	return ret;
 }
 
-void blacklist_remove_bss(const uint8_t *addr)
+void blacklist_remove_bss(const uint8_t *addr, enum blacklist_reason reason)
 {
 	struct blacklist_entry *entry;
+	struct blacklist_search search = {
+		.addr = addr,
+		.reason = reason
+	};
 
 	blacklist_prune();
 
-	entry = l_queue_remove_if(blacklist, match_addr, addr);
+	entry = l_queue_remove_if(blacklist, match_addr, &search);
 
 	if (!entry)
 		return;
diff --git a/src/blacklist.h b/src/blacklist.h
index 56260e20..d4da4478 100644
--- a/src/blacklist.h
+++ b/src/blacklist.h
@@ -20,6 +20,14 @@
  *
  */
 
-void blacklist_add_bss(const uint8_t *addr);
-bool blacklist_contains_bss(const uint8_t *addr);
-void blacklist_remove_bss(const uint8_t *addr);
+enum blacklist_reason {
+	/*
+	 * When a BSS is blacklisted using this reason IWD will refuse to
+	 * connect to it via autoconnect
+	 */
+	BLACKLIST_REASON_PERMANENT,
+};
+
+void blacklist_add_bss(const uint8_t *addr, enum blacklist_reason reason);
+bool blacklist_contains_bss(const uint8_t *addr, enum blacklist_reason reason);
+void blacklist_remove_bss(const uint8_t *addr, enum blacklist_reason reason);
diff --git a/src/network.c b/src/network.c
index 0a40a6c5..92b44ed3 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1280,7 +1280,8 @@ struct scan_bss *network_bss_select(struct network *network,
 		if (l_queue_find(network->blacklist, match_bss, bss))
 			continue;
 
-		if (blacklist_contains_bss(bss->addr))
+		if (blacklist_contains_bss(bss->addr,
+						BLACKLIST_REASON_PERMANENT))
 			continue;
 
 		/* OWE Transition BSS */
diff --git a/src/station.c b/src/station.c
index 5403c332..fab37478 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2880,7 +2880,8 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list,
 		if (network_can_connect_bss(network, bss) < 0)
 			goto next;
 
-		if (blacklist_contains_bss(bss->addr))
+		if (blacklist_contains_bss(bss->addr,
+						BLACKLIST_REASON_PERMANENT))
 			goto next;
 
 		rank = bss->rank;
@@ -3400,7 +3401,8 @@ static bool station_retry_with_reason(struct station *station,
 		break;
 	}
 
-	blacklist_add_bss(station->connected_bss->addr);
+	blacklist_add_bss(station->connected_bss->addr,
+				BLACKLIST_REASON_PERMANENT);
 
 try_next:
 	return station_try_next_bss(station);
@@ -3463,7 +3465,8 @@ static bool station_retry_with_status(struct station *station,
 		network_blacklist_add(station->connected_network,
 						station->connected_bss);
 	else if (!station_pmksa_fallback(station, status_code))
-		blacklist_add_bss(station->connected_bss->addr);
+		blacklist_add_bss(station->connected_bss->addr,
+					BLACKLIST_REASON_PERMANENT);
 
 	iwd_notice(IWD_NOTICE_CONNECT_FAILED, "status: %u", status_code);
 
@@ -3549,7 +3552,8 @@ static void station_connect_cb(struct netdev *netdev, enum netdev_result result,
 
 	switch (result) {
 	case NETDEV_RESULT_OK:
-		blacklist_remove_bss(station->connected_bss->addr);
+		blacklist_remove_bss(station->connected_bss->addr,
+					BLACKLIST_REASON_PERMANENT);
 		station_connect_ok(station);
 		return;
 	case NETDEV_RESULT_DISCONNECTED:
-- 
2.34.1

[PATCH 2/8] blacklist: fix pruning to remove the entry if its expired

[James Prestwood]

When pruning the list check_if_expired was comparing to the maximum
amount of time a BSS can be blacklisted, not if the current time had
exceeded the expirationt time. This results in blacklist entries
hanging around longer than they should, which would result in them
poentially being blacklisted even longer if there was another reason
to blacklist in the future.

Instead on prune check the actual expiration and remove the entry if
its expired. Doing this removes the need to check any of the times
in blacklist_contains_bss since prune will remove any expired entries
correctly.
---
 src/blacklist.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/src/blacklist.c b/src/blacklist.c
index b6583fdf..12100a07 100644
--- a/src/blacklist.c
+++ b/src/blacklist.c
@@ -96,7 +96,7 @@ static bool check_if_expired(void *data, void *user_data)
 	struct blacklist_entry *entry = data;
 	uint64_t now = l_get_u64(user_data);
 
-	if (l_time_diff(now, entry->added_time) > blacklist_max_timeout) {
+	if (l_time_after(now, entry->expire_time)) {
 		l_debug("Removing entry "MAC" on prune", MAC_STR(entry->addr));
 		l_free(entry);
 		return true;
@@ -159,9 +159,6 @@ void blacklist_add_bss(const uint8_t *addr, enum blacklist_reason reason)
 
 bool blacklist_contains_bss(const uint8_t *addr, enum blacklist_reason reason)
 {
-	bool ret;
-	uint64_t time_now;
-	struct blacklist_entry *entry;
 	struct blacklist_search search = {
 		.addr = addr,
 		.reason = reason
@@ -169,16 +166,7 @@ bool blacklist_contains_bss(const uint8_t *addr, enum blacklist_reason reason)
 
 	blacklist_prune();
 
-	entry = l_queue_find(blacklist, match_addr, &search);
-
-	if (!entry)
-		return false;
-
-	time_now = l_time_now();
-
-	ret = l_time_after(time_now, entry->expire_time) ? false : true;
-
-	return ret;
+	return l_queue_find(blacklist, match_addr, &search) != NULL;
 }
 
 void blacklist_remove_bss(const uint8_t *addr, enum blacklist_reason reason)
-- 
2.34.1

[PATCH 3/8] blacklist: add BLACKLIST_REASON_TEMPORARY

[James Prestwood]

This is meant to replace the blacklist held in network objects,
known as the temporary blacklist. For these entires there is no
expiration as it will be up to network.c to remove them as it does
now internally.
---
 src/blacklist.c | 13 +++++++++++++
 src/blacklist.h |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/src/blacklist.c b/src/blacklist.c
index 12100a07..eef3f730 100644
--- a/src/blacklist.c
+++ b/src/blacklist.c
@@ -76,6 +76,16 @@ static struct blacklist_entry *blacklist_entry_new(const uint8_t *addr,
 		added = l_time_now();
 		expires = l_time_offset(added, blacklist_initial_timeout);
 		break;
+	case BLACKLIST_REASON_TEMPORARY:
+		/*
+		 * The temporary blacklist is a special case where entries are
+		 * required to be removed manually. This type of blacklist is
+		 * only used for an ongoing connection attempt to iterate BSS's
+		 * and not retry until all have been exhausted.
+		 */
+		added = 0;
+		expires = 0;
+		break;
 	default:
 		l_warn("Unhandled blacklist reason: %u", reason);
 		return NULL;
@@ -96,6 +106,9 @@ static bool check_if_expired(void *data, void *user_data)
 	struct blacklist_entry *entry = data;
 	uint64_t now = l_get_u64(user_data);
 
+	if (entry->reason == BLACKLIST_REASON_TEMPORARY)
+		return false;
+
 	if (l_time_after(now, entry->expire_time)) {
 		l_debug("Removing entry "MAC" on prune", MAC_STR(entry->addr));
 		l_free(entry);
diff --git a/src/blacklist.h b/src/blacklist.h
index d4da4478..6ce26aba 100644
--- a/src/blacklist.h
+++ b/src/blacklist.h
@@ -26,6 +26,13 @@ enum blacklist_reason {
 	 * connect to it via autoconnect
 	 */
 	BLACKLIST_REASON_PERMANENT,
+	/*
+	 * When a BSS is blacklisted due to a specific subset of error codes.
+	 * This reason is somewhat of a special case and has no expiration. It
+	 * is assumed that the calling module will remove these entries when
+	 * appropriate (after a connection/disconnection)
+	 */
+	BLACKLIST_REASON_TEMPORARY,
 };
 
 void blacklist_add_bss(const uint8_t *addr, enum blacklist_reason reason);
-- 
2.34.1

[PATCH 4/8] network: update to use blacklist's new temporary type

[James Prestwood]

Remove the temporary blacklist from network.c and use the new
BLACKLIST_REASON_TEMPORARY type.
---
 src/network.c | 34 +++++++++++++++++-----------------
 src/network.h |  2 --
 src/station.c |  8 ++++----
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/src/network.c b/src/network.c
index 92b44ed3..b48eaa51 100644
--- a/src/network.c
+++ b/src/network.c
@@ -78,7 +78,6 @@ struct network {
 	struct l_queue *bss_list;
 	struct l_settings *settings;
 	struct l_queue *secrets;
-	struct l_queue *blacklist; /* temporary blacklist for BSS's */
 	uint8_t hessid[6];
 	char **nai_realms;
 	uint8_t *rc_ie;
@@ -168,6 +167,18 @@ static bool network_secret_check_cacheable(void *data, void *user_data)
 	return false;
 }
 
+static void remove_temporary_blacklist(void *user_data)
+{
+	struct scan_bss *bss = user_data;
+
+	blacklist_remove_bss(bss->addr, BLACKLIST_REASON_TEMPORARY);
+}
+
+static void remove_blacklist_foreach(void *data, void *user_data)
+{
+	remove_temporary_blacklist(data);
+}
+
 void network_connected(struct network *network)
 {
 	enum security security = network_get_security(network);
@@ -198,7 +209,7 @@ void network_connected(struct network *network)
 	l_queue_foreach_remove(network->secrets,
 				network_secret_check_cacheable, network);
 
-	l_queue_clear(network->blacklist, NULL);
+	l_queue_foreach(network->bss_list, remove_blacklist_foreach, NULL);
 
 	network->provisioning_hidden = false;
 }
@@ -207,7 +218,7 @@ void network_disconnected(struct network *network)
 {
 	network_settings_close(network);
 
-	l_queue_clear(network->blacklist, NULL);
+	l_queue_foreach(network->bss_list, remove_blacklist_foreach, NULL);
 
 	if (network->provisioning_hidden)
 		station_hide_network(network->station, network);
@@ -254,7 +265,6 @@ struct network *network_create(struct station *station, const char *ssid,
 	}
 
 	network->bss_list = l_queue_new();
-	network->blacklist = l_queue_new();
 
 	return network;
 }
@@ -1197,11 +1207,6 @@ struct scan_bss *network_bss_find_by_addr(struct network *network,
 	return l_queue_find(network->bss_list, match_addr, addr);
 }
 
-static bool match_bss(const void *a, const void *b)
-{
-	return a == b;
-}
-
 struct erp_cache_entry *network_get_erp_cache(struct network *network)
 {
 	struct erp_cache_entry *cache;
@@ -1277,7 +1282,8 @@ struct scan_bss *network_bss_select(struct network *network,
 			candidate = bss;
 
 		/* check if temporarily blacklisted */
-		if (l_queue_find(network->blacklist, match_bss, bss))
+		if (blacklist_contains_bss(bss->addr,
+						BLACKLIST_REASON_TEMPORARY))
 			continue;
 
 		if (blacklist_contains_bss(bss->addr,
@@ -1784,11 +1790,6 @@ struct l_dbus_message *network_connect_new_hidden_network(
 	return dbus_error_not_supported(message);
 }
 
-void network_blacklist_add(struct network *network, struct scan_bss *bss)
-{
-	l_queue_push_head(network->blacklist, bss);
-}
-
 static bool network_property_get_name(struct l_dbus *dbus,
 					struct l_dbus_message *message,
 					struct l_dbus_message_builder *builder,
@@ -1934,8 +1935,7 @@ void network_remove(struct network *network, int reason)
 	if (network->info)
 		network->info->seen_count -= 1;
 
-	l_queue_destroy(network->bss_list, NULL);
-	l_queue_destroy(network->blacklist, NULL);
+	l_queue_destroy(network->bss_list, remove_temporary_blacklist);
 
 	if (network->nai_realms)
 		l_strv_free(network->nai_realms);
diff --git a/src/network.h b/src/network.h
index 849051dd..1e01de88 100644
--- a/src/network.h
+++ b/src/network.h
@@ -95,8 +95,6 @@ struct l_dbus_message *network_connect_new_hidden_network(
 						struct network *network,
 						struct l_dbus_message *message);
 
-void network_blacklist_add(struct network *network, struct scan_bss *bss);
-
 struct erp_cache_entry *network_get_erp_cache(struct network *network);
 
 const struct l_queue_entry *network_bss_list_get_entries(
diff --git a/src/station.c b/src/station.c
index fab37478..d16e82af 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3462,8 +3462,8 @@ static bool station_retry_with_status(struct station *station,
 	 *       obtain that IE, but this should be done in the future.
 	 */
 	if (IS_TEMPORARY_STATUS(status_code))
-		network_blacklist_add(station->connected_network,
-						station->connected_bss);
+		blacklist_add_bss(station->connected_bss->addr,
+					BLACKLIST_REASON_TEMPORARY);
 	else if (!station_pmksa_fallback(station, status_code))
 		blacklist_add_bss(station->connected_bss->addr,
 					BLACKLIST_REASON_PERMANENT);
@@ -3562,8 +3562,8 @@ static void station_connect_cb(struct netdev *netdev, enum netdev_result result,
 		iwd_notice(IWD_NOTICE_DISCONNECT_INFO, "reason: %u", reason);
 
 		/* Disconnected while connecting */
-		network_blacklist_add(station->connected_network,
-						station->connected_bss);
+		blacklist_add_bss(station->connected_bss->addr,
+					BLACKLIST_REASON_TEMPORARY);
 		if (station_try_next_bss(station))
 			return;
 
-- 
2.34.1

[PATCH 5/8] blacklist: add new blacklist reason, ROAM_REQUESTED

[James Prestwood]

This adds a new blacklist reason as well as an option to configure
the timeout. This blacklist reason will be used in cases where a
BSS has requested IWD roam elsewhere. At that time a new blacklist
entry will be added which will be used along with some other criteria
to determine if IWD should connect/roam to that BSS again.
---
 src/blacklist.c | 16 ++++++++++++++++
 src/blacklist.h |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/src/blacklist.c b/src/blacklist.c
index eef3f730..178cb41c 100644
--- a/src/blacklist.c
+++ b/src/blacklist.c
@@ -45,6 +45,7 @@
 
 static uint64_t blacklist_multiplier;
 static uint64_t blacklist_initial_timeout;
+static uint64_t blacklist_roam_initial_timeout;
 static uint64_t blacklist_max_timeout;
 
 struct blacklist_entry {
@@ -86,6 +87,13 @@ static struct blacklist_entry *blacklist_entry_new(const uint8_t *addr,
 		added = 0;
 		expires = 0;
 		break;
+	case BLACKLIST_REASON_ROAM_REQUESTED:
+		if (!blacklist_roam_initial_timeout)
+			return NULL;
+
+		added = l_time_now();
+		expires = l_time_offset(added, blacklist_roam_initial_timeout);
+		break;
 	default:
 		l_warn("Unhandled blacklist reason: %u", reason);
 		return NULL;
@@ -211,6 +219,14 @@ static int blacklist_init(void)
 	/* For easier user configuration the timeout values are in seconds */
 	blacklist_initial_timeout *= L_USEC_PER_SEC;
 
+	if (!l_settings_get_uint64(config, "Blacklist",
+					"InitialRoamRequestedTimeout",
+					&blacklist_roam_initial_timeout))
+		blacklist_roam_initial_timeout = BLACKLIST_DEFAULT_TIMEOUT;
+
+	/* For easier user configuration the timeout values are in seconds */
+	blacklist_roam_initial_timeout *= L_USEC_PER_SEC;
+
 	if (!l_settings_get_uint64(config, "Blacklist",
 					"Multiplier",
 					&blacklist_multiplier))
diff --git a/src/blacklist.h b/src/blacklist.h
index 6ce26aba..ec4c6de3 100644
--- a/src/blacklist.h
+++ b/src/blacklist.h
@@ -33,6 +33,13 @@ enum blacklist_reason {
 	 * appropriate (after a connection/disconnection)
 	 */
 	BLACKLIST_REASON_TEMPORARY,
+	/*
+	 * This type of blacklist is added when a BSS requests IWD roams
+	 * elsewhere. This is to aid in preventing IWD from roaming/connecting
+	 * back to that BSS in the future unless there are no other "good"
+	 * candidates to connect to.
+	 */
+	BLACKLIST_REASON_ROAM_REQUESTED,
 };
 
 void blacklist_add_bss(const uint8_t *addr, enum blacklist_reason reason);
-- 
2.34.1

[PATCH 6/8] scan: Introduce higher level scan BSS groups

[James Prestwood]

This introduces a higher level grouping to BSS's to improve sorting
as opposed to purely rank based sorting. The reason for this is to
handle roam request-based blacklisting where we want to encourage
IWD to avoid BSS's that have requested we roam elsewhere, but also
not fully ban these BSS's (i.e. BLACKLIST_REASON_PERMANENT).

This new concept introduces 4 group types, in order of worse to
best:
 - Blacklisted - the BSS has a permanent timeout blacklist
 - Under threshold - the BSS is under the set RSSI threshold
 - Above threshold - the BSS is above the set RSSI threshold
 - Optimal - The BSS is not "roam blacklisted" and is above the
             set RSSI threshold.

When sorting the BSS group will be considered before rank. This
means we will still prefer roam blacklisted BSS's above those that
are under the set RSSI threshold. BSS's within the same group are
still compared by their rank/signal.

The new group sorting logic was added via a macro
__scan_bss_rank_compare. This was done since station uses a separate
roam_bss structure to sort roam candidates which can also take
advantage of this new sorting. The macro is agnostic to the type as
long as the structure member names match.
---
 src/scan.c | 42 +++++++++++++++++++++++++++++++++++++-----
 src/scan.h | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index aeab6516..2787028d 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -51,6 +51,7 @@
 #include "src/mpdu.h"
 #include "src/band.h"
 #include "src/scan.h"
+#include "src/blacklist.h"
 
 /* User configurable options */
 static double RANK_2G_FACTOR;
@@ -58,6 +59,7 @@ static double RANK_5G_FACTOR;
 static double RANK_6G_FACTOR;
 static uint32_t RANK_HIGH_UTILIZATION;
 static uint32_t RANK_HIGH_STATION_COUNT;
+static int RANK_OPTIMAL_SIGNAL_THRESHOLD;
 static uint32_t SCAN_MAX_INTERVAL;
 static uint32_t SCAN_INIT_INTERVAL;
 
@@ -1910,15 +1912,41 @@ int scan_bss_get_security(const struct scan_bss *bss, enum security *security)
 	return 0;
 }
 
+/*
+ * Evaluate the BSS's grouping based on its signal strength and blacklist
+ * status. From best to worst the groupings are:
+ *
+ * Optimal: Not blacklisted in any form, and above the RSSI threshold
+ * Above Threshold: Above the RSSI threshold (may be roam blacklisted)
+ * Below Threshold: Below the RSSI threshold (may be roam blacklisted)
+ * Blacklisted: Permanently blacklisted
+ */
+enum scan_bss_group scan_bss_evaluate_group(const uint8_t *addr,
+						int16_t signal_strength)
+{
+	int rssi = signal_strength / 100;
+
+	if (RANK_OPTIMAL_SIGNAL_THRESHOLD == 0)
+		return SCAN_BSS_GROUP_OPTIMAL;
+
+	if (blacklist_contains_bss(addr, BLACKLIST_REASON_PERMANENT))
+		return SCAN_BSS_GROUP_BLACKLISTED;
+
+	if (!blacklist_contains_bss(addr, BLACKLIST_REASON_ROAM_REQUESTED) &&
+			rssi >= RANK_OPTIMAL_SIGNAL_THRESHOLD)
+		return SCAN_BSS_GROUP_OPTIMAL;
+
+	if (rssi >= RANK_OPTIMAL_SIGNAL_THRESHOLD)
+		return SCAN_BSS_GROUP_ABOVE_THRESHOLD;
+
+	return SCAN_BSS_GROUP_UNDER_THRESHOLD;
+}
+
 int scan_bss_rank_compare(const void *a, const void *b, void *user_data)
 {
 	const struct scan_bss *new_bss = a, *bss = b;
 
-	if (bss->rank == new_bss->rank)
-		return (bss->signal_strength >
-					new_bss->signal_strength) ? 1 : -1;
-
-	return (bss->rank > new_bss->rank) ? 1 : -1;
+	return __scan_bss_rank_compare(new_bss, bss);
 }
 
 static bool scan_survey_get_snr(struct scan_results *results,
@@ -2678,6 +2706,10 @@ static int scan_init(void)
 	if (L_WARN_ON(RANK_HIGH_STATION_COUNT > 255))
 		RANK_HIGH_STATION_COUNT = 255;
 
+	if (!l_settings_get_int(config, "Rank", "OptimalSignalThreshold",
+					&RANK_OPTIMAL_SIGNAL_THRESHOLD))
+		RANK_OPTIMAL_SIGNAL_THRESHOLD = 0;
+
 	return 0;
 }
 
diff --git a/src/scan.h b/src/scan.h
index 4c1ebc21..4d06f29d 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -45,6 +45,17 @@ enum scan_bss_frame_type {
 	SCAN_BSS_BEACON,
 };
 
+/*
+ * Groupings for BSS's. These are assumed to be in order of preference where
+ * the last enum is the most preferred group to connect to.
+ */
+enum scan_bss_group {
+	SCAN_BSS_GROUP_BLACKLISTED,
+	SCAN_BSS_GROUP_UNDER_THRESHOLD,
+	SCAN_BSS_GROUP_ABOVE_THRESHOLD,
+	SCAN_BSS_GROUP_OPTIMAL,
+};
+
 struct scan_bss {
 	uint8_t addr[6];
 	uint32_t frequency;
@@ -168,6 +179,36 @@ bool scan_get_firmware_scan(uint64_t wdev_id, scan_notify_func_t notify,
 				void *userdata, scan_destroy_func_t destroy);
 
 void scan_bss_free(struct scan_bss *bss);
+
+enum scan_bss_group scan_bss_evaluate_group(const uint8_t *addr,
+						int16_t signal_strength);
+
+/*
+ * Macro to compare two scan_bss-like objects. This is to share code between
+ * station.c and scan.c since station uses "roam_bss" objects which is a subset
+ * of a scan_bss.
+ *  - If the groups differ this is used as the comparison.
+ *  - If the groups match, the BSS rank is used as the comparison
+ *  - If the BSS ranks match, the signal strength is used as the comparison
+ */
+#define __scan_bss_rank_compare(a, b) ({			\
+	int ret;						\
+	enum scan_bss_group a_group = scan_bss_evaluate_group(	\
+			(a)->addr, (a)->signal_strength);	\
+	enum scan_bss_group b_group = scan_bss_evaluate_group(	\
+			(b)->addr, (b)->signal_strength);	\
+	if (b_group > a_group)					\
+		ret = 1;					\
+	else if (b_group < a_group)				\
+		ret = -1;					\
+	else if ((b)->rank == (a)->rank)			\
+		ret = ((b)->signal_strength > 			\
+				(a)->signal_strength) ? 1 : -1;	\
+	else							\
+		ret = ((b)->rank > (a)->rank) ? 1 : -1;		\
+	ret;							\
+})
+
 int scan_bss_rank_compare(const void *a, const void *b, void *user);
 
 int scan_bss_get_rsn_info(const struct scan_bss *bss, struct ie_rsn_info *info);
-- 
2.34.1

[PATCH 7/8] station: roam blacklist BSS when a roam is requested

[James Prestwood]

If the BSS is requesting IWD roam elsewhere add this BSS to the
blacklist using BLACKLIST_REASON_ROAM_REQUESTED. This will lower
the chances of IWD roaming/connecting back to this BSS in the
future.

In addition we also needed to update the roam_bss sorting to use
__scan_bss_rank_compare so we sort based on the new groupings
rather than only rank/rssi.
---
 src/station.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/station.c b/src/station.c
index d16e82af..148f628d 100644
--- a/src/station.c
+++ b/src/station.c
@@ -184,11 +184,7 @@ static int roam_bss_rank_compare(const void *a, const void *b, void *user_data)
 {
 	const struct roam_bss *new_bss = a, *bss = b;
 
-	if (bss->rank == new_bss->rank)
-		return (bss->signal_strength >
-					new_bss->signal_strength) ? 1 : -1;
-
-	return (bss->rank > new_bss->rank) ? 1 : -1;
+	return __scan_bss_rank_compare(new_bss, bss);
 }
 
 struct wiphy *station_get_wiphy(struct station *station)
@@ -3268,6 +3264,10 @@ static void station_ap_directed_roam(struct station *station,
 	l_timeout_remove(station->roam_trigger_timeout);
 	station->roam_trigger_timeout = NULL;
 
+	blacklist_add_bss(station->connected_bss->addr,
+				BLACKLIST_REASON_ROAM_REQUESTED);
+	station_debug_event(station, "ap-roam-blacklist-added");
+
 	if (req_mode & WNM_REQUEST_MODE_PREFERRED_CANDIDATE_LIST) {
 		l_debug("roam: AP sent a preferred candidate list");
 		station_neighbor_report_cb(station->netdev, 0, body + pos,
-- 
2.34.1

[PATCH 8/8] auto-t: add test for AP roam blacklisting

[James Prestwood]

---
 autotests/testAPRoam/connection_test.py     |   2 +-
 autotests/testAPRoam/hw.conf                |   2 +
 autotests/testAPRoam/main.conf              |   5 +
 autotests/testAPRoam/roam_blacklist_test.py | 154 ++++++++++++++++++++
 4 files changed, 162 insertions(+), 1 deletion(-)
 create mode 100644 autotests/testAPRoam/main.conf
 create mode 100644 autotests/testAPRoam/roam_blacklist_test.py

diff --git a/autotests/testAPRoam/connection_test.py b/autotests/testAPRoam/connection_test.py
index a419f4aa..9d17ca87 100644
--- a/autotests/testAPRoam/connection_test.py
+++ b/autotests/testAPRoam/connection_test.py
@@ -13,7 +13,7 @@ from hostapd import HostapdCLI
 class Test(unittest.TestCase):
 
     def validate(self, expect_roam=True):
-        wd = IWD()
+        wd = IWD(True)
 
         devices = wd.list_devices(1)
         device = devices[0]
diff --git a/autotests/testAPRoam/hw.conf b/autotests/testAPRoam/hw.conf
index 00a31063..46b1d4a8 100644
--- a/autotests/testAPRoam/hw.conf
+++ b/autotests/testAPRoam/hw.conf
@@ -1,5 +1,7 @@
 [SETUP]
 num_radios=4
+hwsim_medium=true
+start_iwd=false
 
 [HOSTAPD]
 rad0=ssid1.conf
diff --git a/autotests/testAPRoam/main.conf b/autotests/testAPRoam/main.conf
new file mode 100644
index 00000000..9c9fb994
--- /dev/null
+++ b/autotests/testAPRoam/main.conf
@@ -0,0 +1,5 @@
+[Rank]
+OptimalSignalThreshold=-72
+
+[Blacklist]
+InitialRoamRequestedTimeout=20
diff --git a/autotests/testAPRoam/roam_blacklist_test.py b/autotests/testAPRoam/roam_blacklist_test.py
new file mode 100644
index 00000000..259f6aa5
--- /dev/null
+++ b/autotests/testAPRoam/roam_blacklist_test.py
@@ -0,0 +1,154 @@
+#!/usr/bin/python3
+
+import unittest
+import sys
+
+sys.path.append('../util')
+import iwd
+from iwd import IWD
+from iwd import NetworkType
+
+from hostapd import HostapdCLI
+from hwsim import Hwsim
+
+class Test(unittest.TestCase):
+    def validate_connected(self, hostapd):
+        ordered_network = self.device.get_ordered_network('TestAPRoam')
+
+        self.assertEqual(ordered_network.type, NetworkType.psk)
+
+        condition = 'not obj.connected'
+        self.wd.wait_for_object_condition(ordered_network.network_object, condition)
+
+        self.device.connect_bssid(hostapd.bssid)
+
+        condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_condition(self.device, condition)
+
+        hostapd.wait_for_event('AP-STA-CONNECTED')
+
+    def validate_ap_roamed(self, from_hostapd, to_hostapd):
+        from_hostapd.send_bss_transition(
+            self.device.address, self.neighbor_list, disassoc_imminent=True
+        )
+
+        from_condition = 'obj.state == DeviceState.roaming'
+        to_condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_change(self.device, from_condition, to_condition)
+
+        to_hostapd.wait_for_event('AP-STA-CONNECTED %s' % self.device.address)
+
+        self.device.wait_for_event("ap-roam-blacklist-added")
+
+    def test_roam_to_optimal_candidates(self):
+        # In this test IWD will naturally transition down the list after each
+        # BSS gets roam blacklisted. All BSS's are above the RSSI thresholds.
+        self.rule_ssid1.signal = -5000
+        self.rule_ssid2.signal = -6500
+        self.rule_ssid3.signal = -6900
+
+        # Connect to BSS0
+        self.validate_connected(self.bss_hostapd[0])
+
+        # AP directed roam to BSS1
+        self.validate_ap_roamed(self.bss_hostapd[0], self.bss_hostapd[1])
+
+        # AP directed roam to BSS2
+        self.validate_ap_roamed(self.bss_hostapd[1], self.bss_hostapd[2])
+
+    def test_avoiding_under_threshold_bss(self):
+        # In this test IWD will blacklist BSS0, then roam the BSS1. BSS1 will
+        # then tell IWD to roam, but it should go back to BSS0 since the only
+        # non-blacklisted BSS is under the roam threshold.
+        self.rule_ssid1.signal = -5000
+        self.rule_ssid2.signal = -6500
+        self.rule_ssid3.signal = -7300
+
+        # Connect to BSS0
+        self.validate_connected(self.bss_hostapd[0])
+
+        # AP directed roam to BSS1
+        self.validate_ap_roamed(self.bss_hostapd[0], self.bss_hostapd[1])
+
+        # AP directed roam, but IWD should choose BSS0 since BSS2 is -73dB
+        self.validate_ap_roamed(self.bss_hostapd[1], self.bss_hostapd[0])
+
+    def test_connect_to_roam_blacklisted_bss(self):
+        # In this test a BSS will be roam blacklisted, but all other options are
+        # below the RSSI threshold so IWD should roam back to the blacklisted
+        # BSS.
+        self.rule_ssid1.signal = -5000
+        self.rule_ssid2.signal = -8000
+        self.rule_ssid3.signal = -8500
+
+        # Connect to BSS0
+        self.validate_connected(self.bss_hostapd[0])
+
+        # AP directed roam, should connect to BSS1 as its the next best
+        self.validate_ap_roamed(self.bss_hostapd[0], self.bss_hostapd[1])
+
+        # Connected to BSS1, but the signal is bad, so IWD should try to roam
+        # again. BSS0 is still blacklisted, but its the only reasonable option
+        # since both BSS1 and BSS2 are below the set RSSI threshold (-72dB)
+
+        from_condition = 'obj.state == DeviceState.roaming'
+        to_condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_change(self.device, from_condition, to_condition)
+
+        # IWD should have connected to BSS0, even though its roam blacklisted
+        self.bss_hostapd[0].wait_for_event('AP-STA-CONNECTED %s' % self.device.address)
+
+    def setUp(self):
+        self.wd = IWD(True)
+
+        devices = self.wd.list_devices(1)
+        self.device = devices[0]
+
+
+    def tearDown(self):
+        self.wd = None
+        self.device = None
+
+
+    @classmethod
+    def setUpClass(cls):
+        IWD.copy_to_storage('TestAPRoam.psk')
+        hwsim = Hwsim()
+
+        cls.bss_hostapd = [ HostapdCLI(config='ssid1.conf'),
+                            HostapdCLI(config='ssid2.conf'),
+                            HostapdCLI(config='ssid3.conf') ]
+        HostapdCLI.group_neighbors(*cls.bss_hostapd)
+
+        rad0 = hwsim.get_radio('rad0')
+        rad1 = hwsim.get_radio('rad1')
+        rad2 = hwsim.get_radio('rad2')
+
+        cls.neighbor_list = [
+            (cls.bss_hostapd[0].bssid, "8f0000005101060603000000"),
+            (cls.bss_hostapd[1].bssid, "8f0000005102060603000000"),
+            (cls.bss_hostapd[2].bssid, "8f0000005103060603000000"),
+        ]
+
+
+        cls.rule_ssid1 = hwsim.rules.create()
+        cls.rule_ssid1.source = rad0.addresses[0]
+        cls.rule_ssid1.bidirectional = True
+        cls.rule_ssid1.enabled = True
+
+        cls.rule_ssid2 = hwsim.rules.create()
+        cls.rule_ssid2.source = rad1.addresses[0]
+        cls.rule_ssid2.bidirectional = True
+        cls.rule_ssid2.enabled = True
+
+        cls.rule_ssid3 = hwsim.rules.create()
+        cls.rule_ssid3.source = rad2.addresses[0]
+        cls.rule_ssid3.bidirectional = True
+        cls.rule_ssid3.enabled = True
+
+    @classmethod
+    def tearDownClass(cls):
+        IWD.clear_storage()
+
+if __name__ == '__main__':
+    unittest.main(exit=True)
-- 
2.34.1