[PATCH v2 1/3] station: add checks to prevent multiple roam scans

[PATCH v2 1/3] station: add checks to prevent multiple roam scans

[James Prestwood]

Under the following conditions IWD can accidentally trigger a second
roam scan while one is already in progress:

 - A low RSSI condition is met. This starts the roam rearm timer.
 - A packet loss condition is met, which triggers a roam scan.
 - The roam rearm timer fires and starts another roam scan while
   also overwriting the first roam scan ID.
 - Then, if IWD gets disconnected the overwritten roam scan gets
   canceled, and the roam state is cleared which NULL's
   station->connected_network.
 - The initial roam scan results then come in with the assumption
   that IWD is still connected which results in a crash trying to
   reference station->connected_network.

This can be fixed by adding a station_cannot_roam check in the rearm
timer. If IWD is already doing a roam scan station->preparing_roam
should be set which will cause it to return true and stop any further
action.

Aborting (signal 11) [/usr/libexec/iwd]
iwd[426]: ++++++++ backtrace ++++++++
iwd[426]: #0  0x7f858d7b2090 in /lib/x86_64-linux-gnu/libc.so.6
iwd[426]: #1  0x443df7 in network_get_security() at ome/locus/workspace/iwd/src/network.c:287
iwd[426]: #2  0x421fbb in station_roam_scan_notify() at ome/locus/workspace/iwd/src/station.c:2516
iwd[426]: #3  0x43ebc1 in scan_finished() at ome/locus/workspace/iwd/src/scan.c:1861
iwd[426]: #4  0x43ecf2 in get_scan_done() at ome/locus/workspace/iwd/src/scan.c:1891
iwd[426]: #5  0x4cbfe9 in destroy_request() at ome/locus/workspace/iwd/ell/genl.c:676
iwd[426]: #6  0x4cc98b in process_unicast() at ome/locus/workspace/iwd/ell/genl.c:954
iwd[426]: #7  0x4ccd28 in received_data() at ome/locus/workspace/iwd/ell/genl.c:1052
iwd[426]: #8  0x4c79c9 in io_callback() at ome/locus/workspace/iwd/ell/io.c:120
iwd[426]: #9  0x4c62e3 in l_main_iterate() at ome/locus/workspace/iwd/ell/main.c:476
iwd[426]: #10 0x4c6426 in l_main_run() at ome/locus/workspace/iwd/ell/main.c:519
iwd[426]: #11 0x4c6752 in l_main_run_with_signal() at ome/locus/workspace/iwd/ell/main.c:645
iwd[426]: #12 0x405987 in main() at ome/locus/workspace/iwd/src/main.c:600
iwd[426]: #13 0x7f858d793083 in /lib/x86_64-linux-gnu/libc.so.6
iwd[426]: +++++++++++++++++++++++++++
---
 src/station.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/src/station.c b/src/station.c
index 4452e83e..1a69b98a 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2701,6 +2701,28 @@ static void station_start_roam(struct station *station)
 		station_roam_failed(station);
 }
 
+static bool station_cannot_roam(struct station *station)
+{
+	const struct l_settings *config = iwd_get_config();
+	bool disabled;
+
+	/*
+	 * Disable roaming with hardware that can roam automatically. Note this
+	 * is now required for recent kernels which have CQM event support on
+	 * this type of hardware (e.g. brcmfmac).
+	 */
+	if (wiphy_supports_firmware_roam(station->wiphy))
+		return true;
+
+	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
+								&disabled))
+		disabled = false;
+
+	return disabled || station->preparing_roam ||
+				station->state == STATION_STATE_ROAMING ||
+				station->state == STATION_STATE_FT_ROAMING;
+}
+
 static void station_roam_trigger_cb(struct l_timeout *timeout, void *user_data)
 {
 	struct station *station = user_data;
@@ -2710,6 +2732,9 @@ static void station_roam_trigger_cb(struct l_timeout *timeout, void *user_data)
 	l_timeout_remove(station->roam_trigger_timeout);
 	station->roam_trigger_timeout = NULL;
 
+	if (station_cannot_roam(station))
+		return;
+
 	station_start_roam(station);
 }
 
@@ -2735,28 +2760,6 @@ static void station_roam_timeout_rearm(struct station *station, int seconds)
 								station, NULL);
 }
 
-static bool station_cannot_roam(struct station *station)
-{
-	const struct l_settings *config = iwd_get_config();
-	bool disabled;
-
-	/*
-	 * Disable roaming with hardware that can roam automatically. Note this
-	 * is now required for recent kernels which have CQM event support on
-	 * this type of hardware (e.g. brcmfmac).
-	 */
-	if (wiphy_supports_firmware_roam(station->wiphy))
-		return true;
-
-	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
-								&disabled))
-		disabled = false;
-
-	return disabled || station->preparing_roam ||
-				station->state == STATION_STATE_ROAMING ||
-				station->state == STATION_STATE_FT_ROAMING;
-}
-
 #define WNM_REQUEST_MODE_PREFERRED_CANDIDATE_LIST	(1 << 0)
 #define WNM_REQUEST_MODE_DISASSOCIATION_IMMINENT	(1 << 2)
 #define WNM_REQUEST_MODE_TERMINATION_IMMINENT		(1 << 3)
-- 
2.34.3

[PATCH v2 2/3] station: check for FT work in station_cannot_roam

[James Prestwood]

If station has already started FT ensure station_cannot_roam takes
that into account. Since the state has not yet changed it must also
check if the FT work ID is set.
---
 src/station.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/station.c b/src/station.c
index 1a69b98a..ed98a8d8 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2720,7 +2720,8 @@ static bool station_cannot_roam(struct station *station)
 
 	return disabled || station->preparing_roam ||
 				station->state == STATION_STATE_ROAMING ||
-				station->state == STATION_STATE_FT_ROAMING;
+				station->state == STATION_STATE_FT_ROAMING ||
+				station->ft_work.id;
 }
 
 static void station_roam_trigger_cb(struct l_timeout *timeout, void *user_data)
-- 
2.34.3

[PATCH v2 3/3] station: cancel roam timer when FT starts

[James Prestwood]

Past commits should address any potential problems of the timer
firing during FT, but its still good practice to cancel the timer
once it is no longer needed, i.e. once FT has started.
---
 src/station.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/station.c b/src/station.c
index ed98a8d8..381065a2 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2299,6 +2299,11 @@ static bool station_fast_transition(struct station *station,
 	vendor_ies = network_info_get_extra_ies(info, bss, &iov_elems);
 	handshake_state_set_vendor_ies(hs, vendor_ies, iov_elems);
 
+	if (station->roam_trigger_timeout) {
+		l_timeout_remove(station->roam_trigger_timeout);
+		station->roam_trigger_timeout = NULL;
+	}
+
 	/* Both ft_action/ft_authenticate will gate the associate work item */
 	if ((hs->mde[4] & 1))
 		ft_action(netdev_get_ifindex(station->netdev),
-- 
2.34.3

Re: [PATCH v2 1/3] station: add checks to prevent multiple roam scans

[Denis Kenzior]

Hi James,

On 1/13/23 15:24, James Prestwood wrote:
> Under the following conditions IWD can accidentally trigger a second
> roam scan while one is already in progress:
> 
>   - A low RSSI condition is met. This starts the roam rearm timer.
>   - A packet loss condition is met, which triggers a roam scan.
>   - The roam rearm timer fires and starts another roam scan while
>     also overwriting the first roam scan ID.
>   - Then, if IWD gets disconnected the overwritten roam scan gets
>     canceled, and the roam state is cleared which NULL's
>     station->connected_network.
>   - The initial roam scan results then come in with the assumption
>     that IWD is still connected which results in a crash trying to
>     reference station->connected_network.
> 
> This can be fixed by adding a station_cannot_roam check in the rearm
> timer. If IWD is already doing a roam scan station->preparing_roam
> should be set which will cause it to return true and stop any further
> action.
> 

All applied, thanks.

Regards,
-Denis