[v2] station: allow roaming before netconfig finishes

Commit Message

James Prestwood May 2, 2023, 6:59 p.m. UTC

If IWD connects under bad RF conditions and netconfig takes
a while to complete (e.g. slow DHCP), the roam timeout
could fire before DHCP is done. Then, after the roam,
IWD would transition automatically to connected before
DHCP was finished. In theory DHCP could still complete after
this point but any process depending on IWD's connected
state would be uninformed and assume IP networking is up.

Fix this by stopping netconfig prior to a roam if IWD is not
in a connected state. Then, once the roam either failed or
succeeded, start netconfig again.
---
 src/station.c | 151 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 90 insertions(+), 61 deletions(-)

v2:
 * Reworked to allow the roam to finish then start netconfig.
   Now netconfig will be reset and resumed after the roam.

Comments

Denis Kenzior May 7, 2023, 11:06 p.m. UTC | #1

Hi James,

On 5/2/23 13:59, James Prestwood wrote:
> If IWD connects under bad RF conditions and netconfig takes
> a while to complete (e.g. slow DHCP), the roam timeout
> could fire before DHCP is done. Then, after the roam,
> IWD would transition automatically to connected before
> DHCP was finished. In theory DHCP could still complete after
> this point but any process depending on IWD's connected
> state would be uninformed and assume IP networking is up.
> 
> Fix this by stopping netconfig prior to a roam if IWD is not
> in a connected state. Then, once the roam either failed or
> succeeded, start netconfig again.
> ---
>   src/station.c | 151 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 90 insertions(+), 61 deletions(-)
> 
> v2:
>   * Reworked to allow the roam to finish then start netconfig.
>     Now netconfig will be reset and resumed after the roam.
> 

I went ahead and applied this.  But the question still remains whether this was 
tested somehow?  Is there an auto test for this maybe?

Regards,
-Denis

James Prestwood May 8, 2023, 1:51 p.m. UTC | #2

Hi Denis,

On 5/7/23 4:06 PM, Denis Kenzior wrote:
> Hi James,
> 
> On 5/2/23 13:59, James Prestwood wrote:
>> If IWD connects under bad RF conditions and netconfig takes
>> a while to complete (e.g. slow DHCP), the roam timeout
>> could fire before DHCP is done. Then, after the roam,
>> IWD would transition automatically to connected before
>> DHCP was finished. In theory DHCP could still complete after
>> this point but any process depending on IWD's connected
>> state would be uninformed and assume IP networking is up.
>>
>> Fix this by stopping netconfig prior to a roam if IWD is not
>> in a connected state. Then, once the roam either failed or
>> succeeded, start netconfig again.
>> ---
>>   src/station.c | 151 ++++++++++++++++++++++++++++++--------------------
>>   1 file changed, 90 insertions(+), 61 deletions(-)
>>
>> v2:
>>   * Reworked to allow the roam to finish then start netconfig.
>>     Now netconfig will be reset and resumed after the roam.
>>
> 
> I went ahead and applied this.  But the question still remains whether 
> this was tested somehow?  Is there an auto test for this maybe?

I tested this locally, my DHCP server seems really screwed up and takes 
forever (in this case perfect for testing). But adding an autotest was 
planned as well (which will need those hwsim/namespace changes)

> 
> Regards,
> -Denis
>

Patch

diff --git a/src/station.c b/src/station.c
index d5650a29..be571083 100644
--- a/src/station.c
+++ b/src/station.c
@@ -129,6 +129,7 @@  struct station {
 	bool scanning : 1;
 	bool autoconnect : 1;
 	bool autoconnect_can_start : 1;
+	bool netconfig_after_roam : 1;
 };
 
 struct anqp_entry {
@@ -1683,6 +1684,7 @@  static void station_roam_state_clear(struct station *station)
 	station->roam_scan_full = false;
 	station->signal_low = false;
 	station->roam_min_time.tv_sec = 0;
+	station->netconfig_after_roam = false;
 
 	if (station->roam_scan_id)
 		scan_cancel(netdev_get_wdev_id(station->netdev),
@@ -1971,6 +1973,65 @@  static bool station_can_fast_transition(struct handshake_state *hs,
 	return true;
 }
 
+static void station_disconnect_on_error_cb(struct netdev *netdev, bool success,
+					void *user_data)
+{
+	struct station *station = user_data;
+	bool continue_autoconnect;
+
+	station_enter_state(station, STATION_STATE_DISCONNECTED);
+
+	continue_autoconnect = station->state == STATION_STATE_CONNECTING_AUTO;
+
+	if (continue_autoconnect) {
+		if (station_autoconnect_next(station) < 0) {
+			l_debug("Nothing left on autoconnect list");
+			station_enter_state(station,
+					STATION_STATE_AUTOCONNECT_FULL);
+		}
+
+		return;
+	}
+
+	if (station->autoconnect)
+		station_enter_state(station, STATION_STATE_AUTOCONNECT_QUICK);
+}
+
+static void station_netconfig_event_handler(enum netconfig_event event,
+							void *user_data)
+{
+	struct station *station = user_data;
+
+	switch (event) {
+	case NETCONFIG_EVENT_CONNECTED:
+		station_enter_state(station, STATION_STATE_CONNECTED);
+		break;
+	case NETCONFIG_EVENT_FAILED:
+		if (station->connect_pending) {
+			struct l_dbus_message *reply = dbus_error_failed(
+						station->connect_pending);
+
+			dbus_pending_reply(&station->connect_pending, reply);
+		}
+
+		if (L_IN_SET(station->state, STATION_STATE_CONNECTING,
+				STATION_STATE_CONNECTING_AUTO))
+			network_connect_failed(station->connected_network,
+						false);
+
+		netdev_disconnect(station->netdev,
+					station_disconnect_on_error_cb,
+					station);
+		station_reset_connection_state(station);
+
+		station_enter_state(station, STATION_STATE_DISCONNECTING);
+		break;
+	default:
+		l_error("station: Unsupported netconfig event: %d.", event);
+		break;
+	}
+}
+
 static void station_roamed(struct station *station)
 {
 	station->roam_scan_full = false;
@@ -1999,7 +2060,14 @@  static void station_roamed(struct station *station)
 
 	l_queue_clear(station->roam_bss_list, l_free);
 
-	station_enter_state(station, STATION_STATE_CONNECTED);
+	/* Re-enable netconfig if it never finished on the last BSS */
+	if (station->netconfig_after_roam) {
+		station->netconfig_after_roam = false;
+		L_WARN_ON(!netconfig_configure(station->netconfig,
+						station_netconfig_event_handler,
+						station));
+	} else
+		station_enter_state(station, STATION_STATE_CONNECTED);
 }
 
 static void station_roam_retry(struct station *station)
@@ -2032,6 +2100,14 @@  static void station_roam_failed(struct station *station)
 		return;
 	}
 
+	/* Re-enable netconfig if needed, even on a failed roam */
+	if (station->netconfig_after_roam) {
+		station->netconfig_after_roam = false;
+		L_WARN_ON(!netconfig_configure(station->netconfig,
+						station_netconfig_event_handler,
+						station));
+	}
+
 	/*
 	 * We were told by the AP to roam, but failed.  Try ourselves or
 	 * wait for the AP to tell us to roam again
@@ -2060,65 +2136,6 @@  delayed_retry:
 	station_roam_retry(station);
 }
 
-static void station_disconnect_on_error_cb(struct netdev *netdev, bool success,
-					void *user_data)
-{
-	struct station *station = user_data;
-	bool continue_autoconnect;
-
-	station_enter_state(station, STATION_STATE_DISCONNECTED);
-
-	continue_autoconnect = station->state == STATION_STATE_CONNECTING_AUTO;
-
-	if (continue_autoconnect) {
-		if (station_autoconnect_next(station) < 0) {
-			l_debug("Nothing left on autoconnect list");
-			station_enter_state(station,
-					STATION_STATE_AUTOCONNECT_FULL);
-		}
-
-		return;
-	}
-
-	if (station->autoconnect)
-		station_enter_state(station, STATION_STATE_AUTOCONNECT_QUICK);
-}
-
-static void station_netconfig_event_handler(enum netconfig_event event,
-							void *user_data)
-{
-	struct station *station = user_data;
-
-	switch (event) {
-	case NETCONFIG_EVENT_CONNECTED:
-		station_enter_state(station, STATION_STATE_CONNECTED);
-		break;
-	case NETCONFIG_EVENT_FAILED:
-		if (station->connect_pending) {
-			struct l_dbus_message *reply = dbus_error_failed(
-						station->connect_pending);
-
-			dbus_pending_reply(&station->connect_pending, reply);
-		}
-
-		if (L_IN_SET(station->state, STATION_STATE_CONNECTING,
-				STATION_STATE_CONNECTING_AUTO))
-			network_connect_failed(station->connected_network,
-						false);
-
-		netdev_disconnect(station->netdev,
-					station_disconnect_on_error_cb,
-					station);
-		station_reset_connection_state(station);
-
-		station_enter_state(station, STATION_STATE_DISCONNECTING);
-		break;
-	default:
-		l_error("station: Unsupported netconfig event: %d.", event);
-		break;
-	}
-}
-
 static void station_reassociate_cb(struct netdev *netdev,
 					enum netdev_result result,
 					void *event_data,
@@ -2404,8 +2421,20 @@  static void station_transition_start(struct station *station)
 		l_free(rbss);
 	}
 
-	if (!roaming)
+	if (!roaming) {
 		station_roam_failed(station);
+		return;
+	}
+
+	/*
+	 * Netconfig could potentially be running and not completed yet. We
+	 * still should roam in this case but need to restart netconfig once the
+	 * roam is finished.
+	 */
+	if (station->netconfig && station->state != STATION_STATE_CONNECTED) {
+		netconfig_reset(station->netconfig);
+		station->netconfig_after_roam = true;
+	}
 }
 
 static void station_roam_scan_triggered(int err, void *user_data)