[PATCH 1/3] netdev: Remove improper use of netdev_connect_failed

[PATCH 1/3] netdev: Remove improper use of netdev_connect_failed

[Denis Kenzior]

When a roam event is received, iwd generates a firmware scan request and
notifies its event filter of the ROAMING condition.  In cases where the
firmware scan could not be started successfully, netdev_connect_failed
is invoked.  This is not a correct use of netev_connect_failed since it
doesn't actually disconnect the underlying netdev and the reflected
state becomes de-synchronized from the underlying kernel device.

The firmware scan request could currently fail for two reasons:
  1. nl80211 genl socket is in a bad state, or
  2. the scan context does not exist

Since both reasons are highly unlikely, simply use L_WARN instead.

The other two cases where netdev_connect_failed is used could only occur
if the kernel message is invalid.  The message is ignored in that case
and a warning is printed.

The situation described above also exists in netdev_get_fw_scan_cb. If
the scan could not be completed successfully, there's not much iwd can
do to recover.  Have iwd remain in roaming state and print an error.
---
 src/netdev.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index ffd903740dd9..4a418b60abcf 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -4934,7 +4934,7 @@ static bool netdev_get_fw_scan_cb(int err, struct l_queue *bss_list,
 
 	if (err < 0) {
 		l_error("Failed to get scan after roam (%d)", err);
-		goto failed;
+		return false;
 	}
 
 	/*
@@ -4946,7 +4946,7 @@ static bool netdev_get_fw_scan_cb(int err, struct l_queue *bss_list,
 
 	if (!bss) {
 		l_error("Roam target BSS not found in scan results");
-		goto failed;
+		return false;
 	}
 
 	netdev->fw_roam_bss = bss;
@@ -4958,16 +4958,9 @@ static bool netdev_get_fw_scan_cb(int err, struct l_queue *bss_list,
 		return false;
 	}
 
-	if (netdev->sm) {
-		if (!eapol_start(netdev->sm))
-			goto failed;
-	}
-
-	return false;
+	if (netdev->sm)
+		L_WARN_ON(!eapol_start(netdev->sm));
 
-failed:
-	netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
-					MMPDU_REASON_CODE_UNSPECIFIED);
 	return false;
 }
 
@@ -4998,8 +4991,8 @@ static void netdev_roam_event(struct l_genl_msg *msg, struct netdev *netdev)
 
 	netdev->operational = false;
 
-	if (!l_genl_attr_init(&attr, msg))
-		goto failed;
+	if (L_WARN_ON(!l_genl_attr_init(&attr, msg)))
+		return;
 
 	while (l_genl_attr_next(&attr, &type, &len, &data)) {
 		switch (type) {
@@ -5014,7 +5007,7 @@ static void netdev_roam_event(struct l_genl_msg *msg, struct netdev *netdev)
 
 	if (!mac) {
 		l_error("Failed to parse ATTR_MAC from CMD_ROAM");
-		goto failed;
+		return;
 	}
 
 	/* Handshake completed in firmware, just get the roamed BSS */
@@ -5031,20 +5024,14 @@ static void netdev_roam_event(struct l_genl_msg *msg, struct netdev *netdev)
 get_fw_scan:
 	handshake_state_set_authenticator_address(netdev->handshake, mac);
 
-	if (!scan_get_firmware_scan(netdev->wdev_id, netdev_get_fw_scan_cb,
-					netdev, NULL))
-		goto failed;
+	if (L_WARN_ON(!scan_get_firmware_scan(netdev->wdev_id,
+					netdev_get_fw_scan_cb,
+					netdev, NULL)))
+		return;
 
 	if (netdev->event_filter)
 		netdev->event_filter(netdev, NETDEV_EVENT_ROAMING,
 					NULL, netdev->user_data);
-
-	return;
-failed:
-	l_error("Failed to properly handle the ROAM event -- submit logs!");
-	netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
-					MMPDU_REASON_CODE_UNSPECIFIED);
-
 }
 
 static void netdev_send_sa_query_delay(struct l_timeout *timeout,
-- 
2.42.0

[PATCH 2/3] netdev: Simplify netdev_auth_cb error logic

[Denis Kenzior]

---
 src/netdev.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 4a418b60abcf..327be768d3d9 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3240,25 +3240,22 @@ static void netdev_auth_cb(struct l_genl_msg *msg, void *user_data)
 
 	l_debug("Error during auth: %d", err);
 
-	if (!netdev->auth_cmd || err != -ENOENT) {
-		netdev_connect_failed(netdev,
-					NETDEV_RESULT_AUTHENTICATION_FAILED,
-					MMPDU_STATUS_CODE_UNSPECIFIED);
-		return;
-	}
+	if (!netdev->auth_cmd || err != -ENOENT)
+		goto failed;
 
 	/* Kernel can't find the BSS in its cache, scan and retry */
 	scan_msg = scan_build_trigger_scan_bss(netdev->index, netdev->wiphy,
 						netdev->frequency,
 						hs->ssid, hs->ssid_len);
 
-	if (!l_genl_family_send(nl80211, scan_msg,
-					netdev_scan_cb, netdev, NULL)) {
-		l_genl_msg_unref(scan_msg);
-		netdev_connect_failed(netdev,
-					NETDEV_RESULT_AUTHENTICATION_FAILED,
+	if (l_genl_family_send(nl80211, scan_msg,
+					netdev_scan_cb, netdev, NULL) > 0)
+		return;
+
+	l_genl_msg_unref(scan_msg);
+failed:
+	netdev_connect_failed(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
 					MMPDU_STATUS_CODE_UNSPECIFIED);
-	}
 }
 
 static void netdev_new_scan_results_event(struct l_genl_msg *msg,
-- 
2.42.0

[PATCH 3/3] netdev: Separate connect_failed and disconnected paths

[Denis Kenzior]

Commit c59669a366c5 ("netdev: disambiguate between disconnection types")
introduced different paths for different types of disconnection
notifications from netdev.  Formalize this further by having
netdev_connect_failed only invoke connect_cb.

Disconnections that could be triggered outside of connection
related events are now handled on a different code path.  For this
purpose, netdev_disconnected() is introduced.
---
 src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 327be768d3d9..e31a51617671 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -839,7 +839,6 @@ static void netdev_connect_failed(struct netdev *netdev,
 					uint16_t status_or_reason)
 {
 	netdev_connect_cb_t connect_cb = netdev->connect_cb;
-	netdev_event_func_t event_filter = netdev->event_filter;
 	void *connect_data = netdev->user_data;
 
 	/* Done this way to allow re-entrant netdev_connect calls */
@@ -847,15 +846,6 @@ static void netdev_connect_failed(struct netdev *netdev,
 
 	if (connect_cb)
 		connect_cb(netdev, result, &status_or_reason, connect_data);
-	else if (event_filter) {
-		/* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
-		if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
-			status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
-
-		event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
-				&status_or_reason,
-				connect_data);
-	}
 }
 
 static void netdev_connect_failed_cb(struct l_genl_msg *msg, void *user_data)
@@ -900,12 +890,42 @@ static void netdev_deauth_and_fail_connection(struct netdev *netdev,
 	netdev_send_and_fail_connection(netdev, result, status_code, msg);
 }
 
+/*
+ * If we have a connection callback pending, either through netdev_connect
+ * or netdev_reassociate, then invoke that callback with the @result and
+ * @status_or_reason.  Otherwise, invoke the event callback with the @event
+ * and @status_or_reason.
+ *
+ * This is useful for situations where handshaking or setting keys somehow
+ * fails (perhaps due to rekeying), or if the device is removed / brought
+ * down when keys are being set as a result of a rekey
+ */
+static void netdev_disconnected(struct netdev *netdev,
+					enum netdev_result result,
+					enum netdev_event event,
+					uint16_t status_or_reason)
+{
+	netdev_event_func_t event_filter = netdev->event_filter;
+	void *event_data = netdev->user_data;
+
+	if (netdev->connect_cb) {
+		netdev_connect_failed(netdev, result, status_or_reason);
+		return;
+	}
+
+	netdev_connect_free(netdev);
+
+	if (event_filter)
+		event_filter(netdev, event, &status_or_reason, event_data);
+}
+
 static void netdev_disconnect_by_sme_cb(struct l_genl_msg *msg, void *user_data)
 {
 	struct netdev *netdev = user_data;
 
 	netdev->disconnect_cmd_id = 0;
-	netdev_connect_failed(netdev, netdev->result, netdev->last_code);
+	netdev_disconnected(netdev, netdev->result,
+			NETDEV_EVENT_DISCONNECT_BY_SME, netdev->last_code);
 }
 
 static void netdev_disconnect_by_sme(struct netdev *netdev,
@@ -1440,7 +1460,8 @@ static void netdev_setting_keys_failed(struct netdev_handshake_state *nhs,
 		 * CMD_DISCONNECT
 		 */
 		if (err == -ENETDOWN) {
-			netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
+			netdev_disconnected(netdev, NETDEV_RESULT_ABORTED,
+					NETDEV_EVENT_DISCONNECT_BY_SME,
 					MMPDU_STATUS_CODE_UNSPECIFIED);
 			return;
 		}
-- 
2.42.0

Re: [PATCH 3/3] netdev: Separate connect_failed and disconnected paths

[James Prestwood]

Hi Denis,

On 11/14/23 16:05, Denis Kenzior wrote:
> Commit c59669a366c5 ("netdev: disambiguate between disconnection types")
> introduced different paths for different types of disconnection
> notifications from netdev.  Formalize this further by having
> netdev_connect_failed only invoke connect_cb.
>
> Disconnections that could be triggered outside of connection
> related events are now handled on a different code path.  For this
> purpose, netdev_disconnected() is introduced.
> ---
>   src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------
>   1 file changed, 33 insertions(+), 12 deletions(-)

LGTM. You may have already thought of this, but I do still think we 
could have a case where an AP could send a deauth during key setting 
right?. So key setting could still succeed but station would get 
notified of the failure via the netdev event (in 
netdev_disconnect_event), not the connect callback.

> diff --git a/src/netdev.c b/src/netdev.c
> index 327be768d3d9..e31a51617671 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -839,7 +839,6 @@ static void netdev_connect_failed(struct netdev *netdev,
>   					uint16_t status_or_reason)
>   {
>   	netdev_connect_cb_t connect_cb = netdev->connect_cb;
> -	netdev_event_func_t event_filter = netdev->event_filter;
>   	void *connect_data = netdev->user_data;
>   
>   	/* Done this way to allow re-entrant netdev_connect calls */
> @@ -847,15 +846,6 @@ static void netdev_connect_failed(struct netdev *netdev,
>   
>   	if (connect_cb)
>   		connect_cb(netdev, result, &status_or_reason, connect_data);
> -	else if (event_filter) {
> -		/* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
> -		if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
> -			status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
> -
> -		event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
> -				&status_or_reason,
> -				connect_data);
> -	}
>   }
>   
>   static void netdev_connect_failed_cb(struct l_genl_msg *msg, void *user_data)
> @@ -900,12 +890,42 @@ static void netdev_deauth_and_fail_connection(struct netdev *netdev,
>   	netdev_send_and_fail_connection(netdev, result, status_code, msg);
>   }
>   
> +/*
> + * If we have a connection callback pending, either through netdev_connect
> + * or netdev_reassociate, then invoke that callback with the @result and
> + * @status_or_reason.  Otherwise, invoke the event callback with the @event
> + * and @status_or_reason.
> + *
> + * This is useful for situations where handshaking or setting keys somehow
> + * fails (perhaps due to rekeying), or if the device is removed / brought
> + * down when keys are being set as a result of a rekey
> + */
> +static void netdev_disconnected(struct netdev *netdev,
> +					enum netdev_result result,
> +					enum netdev_event event,
> +					uint16_t status_or_reason)
> +{
> +	netdev_event_func_t event_filter = netdev->event_filter;
> +	void *event_data = netdev->user_data;
> +
> +	if (netdev->connect_cb) {
> +		netdev_connect_failed(netdev, result, status_or_reason);
> +		return;
> +	}
> +
> +	netdev_connect_free(netdev);
> +
> +	if (event_filter)
> +		event_filter(netdev, event, &status_or_reason, event_data);
> +}
> +
>   static void netdev_disconnect_by_sme_cb(struct l_genl_msg *msg, void *user_data)
>   {
>   	struct netdev *netdev = user_data;
>   
>   	netdev->disconnect_cmd_id = 0;
> -	netdev_connect_failed(netdev, netdev->result, netdev->last_code);
> +	netdev_disconnected(netdev, netdev->result,
> +			NETDEV_EVENT_DISCONNECT_BY_SME, netdev->last_code);
>   }
>   
>   static void netdev_disconnect_by_sme(struct netdev *netdev,
> @@ -1440,7 +1460,8 @@ static void netdev_setting_keys_failed(struct netdev_handshake_state *nhs,
>   		 * CMD_DISCONNECT
>   		 */
>   		if (err == -ENETDOWN) {
> -			netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
> +			netdev_disconnected(netdev, NETDEV_RESULT_ABORTED,
> +					NETDEV_EVENT_DISCONNECT_BY_SME,
>   					MMPDU_STATUS_CODE_UNSPECIFIED);
>   			return;
>   		}

Re: [PATCH 3/3] netdev: Separate connect_failed and disconnected paths

[Denis Kenzior]

Hi James,

On 11/15/23 12:27, James Prestwood wrote:
> Hi Denis,
> 
> On 11/14/23 16:05, Denis Kenzior wrote:
>> Commit c59669a366c5 ("netdev: disambiguate between disconnection types")
>> introduced different paths for different types of disconnection
>> notifications from netdev.  Formalize this further by having
>> netdev_connect_failed only invoke connect_cb.
>>
>> Disconnections that could be triggered outside of connection
>> related events are now handled on a different code path.  For this
>> purpose, netdev_disconnected() is introduced.
>> ---
>>   src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------
>>   1 file changed, 33 insertions(+), 12 deletions(-)
> 
> LGTM. You may have already thought of this, but I do still think we could have a 
> case where an AP could send a deauth during key setting right?. So key setting 
> could still succeed but station would get notified of the failure via the netdev 
> event (in netdev_disconnect_event), not the connect callback.
> 
Yes, it can happen at any time.  We have some inconsistencies in how we handle 
the corner cases.  For example:

- We Authenticate successfully, but AP Deauthenticates us before we Associate. 
This is handled in netdev_deauthenticate_event.  See 2bebb4bdc7ee ("netdev: 
Handle deauth frames prior to association").  We treat this as a connect 
failure, but I'm no longer sure that is right.
- We associate and handshake successfully, but AP decides to deauthenticate us 
prior to setting keys, or setting station.  Pretty strange case, but should our 
connection throttling logic apply?  Today we treat this case as a connection 
failure for initial connections, and don't handle it at all for the roaming 
case.  This is what is happening in Alvin's case.
- Firmware / kernel can deauthenticate us at any point.  Kernel probably doesn't 
actually do this, but the firmware can?  How should we handle these?


Also, I think we might still have some problems when we connect / roam 
successfully but get disconnected prior to netconfig finishing.  We should audit 
the code and make sure we're consistent.  For example, are d-bus method returns 
and state transitions generated at the right time?

Regards,
-Denis

Re: [PATCH 1/3] netdev: Remove improper use of netdev_connect_failed

[Denis Kenzior]

On 11/14/23 18:05, Denis Kenzior wrote:
> When a roam event is received, iwd generates a firmware scan request and
> notifies its event filter of the ROAMING condition.  In cases where the
> firmware scan could not be started successfully, netdev_connect_failed
> is invoked.  This is not a correct use of netev_connect_failed since it
> doesn't actually disconnect the underlying netdev and the reflected
> state becomes de-synchronized from the underlying kernel device.
> 
> The firmware scan request could currently fail for two reasons:
>    1. nl80211 genl socket is in a bad state, or
>    2. the scan context does not exist
> 
> Since both reasons are highly unlikely, simply use L_WARN instead.
> 
> The other two cases where netdev_connect_failed is used could only occur
> if the kernel message is invalid.  The message is ignored in that case
> and a warning is printed.
> 
> The situation described above also exists in netdev_get_fw_scan_cb. If
> the scan could not be completed successfully, there's not much iwd can
> do to recover.  Have iwd remain in roaming state and print an error.
> ---
>   src/netdev.c | 35 +++++++++++------------------------
>   1 file changed, 11 insertions(+), 24 deletions(-)
> 

All applied.

Regards,
-Denis