[PATCH v3 1/4] netdev: move failure point out of netdev_connect_common

[PATCH v3 1/4] netdev: move failure point out of netdev_connect_common

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3479 bytes --]

The only point of failure in netdev_connect_common was setting
up the handshake type. Moving this outside of netdev_connect_common
makes the code flow much better in netdev_{connect,reassociate} as
nothing needs to be reset upon failure.
---
 src/netdev.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

v3:
 * Fixed previous break to open networks due to netdev_connect_common
   not handling the !is_rsn special case. This check was moved into
   netdev_handshake_state_setup_connection_type.

diff --git a/src/netdev.c b/src/netdev.c
index 1b72b0a7..f9731892 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3364,6 +3364,9 @@ static int netdev_handshake_state_setup_connection_type(
 	bool softmac = wiphy_supports_cmds_auth_assoc(wiphy);
 	bool canroam = wiphy_supports_firmware_roam(wiphy);
 
+	if (hs->supplicant_ie == NULL)
+		goto softmac;
+
 	/*
 	 * Sanity check that any FT AKMs are set only on softmac or on
 	 * devices that support firmware roam
@@ -3442,7 +3445,7 @@ offload_1x:
 	return 0;
 }
 
-static int netdev_connect_common(struct netdev *netdev,
+static void netdev_connect_common(struct netdev *netdev,
 					struct scan_bss *bss,
 					struct scan_bss *prev_bss,
 					struct handshake_state *hs,
@@ -3458,13 +3461,8 @@ static int netdev_connect_common(struct netdev *netdev,
 	bool is_rsn = hs->supplicant_ie != NULL;
 	const uint8_t *prev_bssid = prev_bss ? prev_bss->addr : NULL;
 
-	if (!is_rsn) {
-		nhs->type = CONNECTION_TYPE_SOFTMAC;
+	if (!is_rsn)
 		goto build_cmd_connect;
-	}
-
-	if (netdev_handshake_state_setup_connection_type(hs) < 0)
-		return -ENOTSUP;
 
 	if (nhs->type != CONNECTION_TYPE_SOFTMAC)
 		goto build_cmd_connect;
@@ -3534,7 +3532,7 @@ build_cmd_connect:
 	wiphy_radio_work_insert(netdev->wiphy, &netdev->work, 1,
 				&connect_work_ops);
 
-	return 0;
+	return;
 }
 
 int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
@@ -3554,9 +3552,14 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
 	if (netdev->connected || netdev->connect_cmd_id || netdev->work.id)
 		return -EISCONN;
 
-	return netdev_connect_common(netdev, bss, NULL, hs, vendor_ies,
+	if (netdev_handshake_state_setup_connection_type(hs) < 0)
+		return -ENOTSUP;
+
+	netdev_connect_common(netdev, bss, NULL, hs, vendor_ies,
 					num_vendor_ies, event_filter, cb,
 					user_data);
+
+	return 0;
 }
 
 static void disconnect_idle(struct l_idle *idle, void *user_data)
@@ -3646,24 +3649,24 @@ int netdev_reassociate(struct netdev *netdev, struct scan_bss *target_bss,
 {
 	struct handshake_state *old_hs;
 	struct eapol_sm *old_sm;
-	int ret;
 
 	old_sm = netdev->sm;
 	old_hs = netdev->handshake;
 
-	ret = netdev_connect_common(netdev, target_bss, orig_bss, hs, NULL, 0,
-					event_filter, cb, user_data);
-	if (ret < 0)
-		return ret;
-
-	if (netdev->ap)
-		memcpy(netdev->ap->prev_bssid, orig_bss->addr, ETH_ALEN);
+	if (netdev_handshake_state_setup_connection_type(hs) < 0)
+		return -ENOTSUP;
 
 	netdev->associated = false;
 	netdev->operational = false;
 	netdev->connected = false;
 	netdev->in_reassoc = true;
 
+	netdev_connect_common(netdev, target_bss, orig_bss, hs, NULL, 0,
+					event_filter, cb, user_data);
+
+	if (netdev->ap)
+		memcpy(netdev->ap->prev_bssid, orig_bss->addr, ETH_ALEN);
+
 	netdev_rssi_polling_update(netdev);
 
 	if (old_sm)
-- 
2.31.1

[PATCH v3 2/4] station: add ConnectBssid() developer method

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 6262 bytes --]

This method will initiate a connection to a specific BSS rather
than relying on a network based connection (which the user has
no control over which specific BSS is selected).
---
 src/station.c | 129 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 111 insertions(+), 18 deletions(-)

diff --git a/src/station.c b/src/station.c
index bc4e7539..9a053c85 100644
--- a/src/station.c
+++ b/src/station.c
@@ -286,6 +286,34 @@ static int bss_signal_strength_compare(const void *a, const void *b, void *user)
 	return (bss->signal_strength > new_bss->signal_strength) ? 1 : -1;
 }
 
+static int station_parse_bss(struct station *station,
+				struct scan_bss *bss, char *ssid_out,
+				enum security *security_out,
+				char *path_out)
+{
+	struct ie_rsn_info info;
+	const char *path;
+	int r;
+
+	r = scan_bss_get_rsn_info(bss, &info);
+	if (r < 0) {
+		if (r != -ENOENT)
+			return r;
+
+		*security_out = security_determine(bss->capability, NULL);
+	} else
+		*security_out = security_determine(bss->capability, &info);
+
+	memcpy(ssid_out, bss->ssid, bss->ssid_len);
+	ssid_out[bss->ssid_len] = '\0';
+
+	path = iwd_network_get_path(station, ssid_out, *security_out);
+
+	strcpy(path_out, path);
+
+	return 0;
+}
+
 /*
  * Returns the network object the BSS was added to or NULL if ignored.
  */
@@ -293,10 +321,8 @@ static struct network *station_add_seen_bss(struct station *station,
 						struct scan_bss *bss)
 {
 	struct network *network;
-	struct ie_rsn_info info;
-	int r;
 	enum security security;
-	const char *path;
+	char path[256];
 	char ssid[33];
 	uint32_t kbps100 = DIV_ROUND_CLOSEST(bss->data_rate, 100000);
 
@@ -315,25 +341,13 @@ static struct network *station_add_seen_bss(struct station *station,
 		return NULL;
 	}
 
-	memcpy(ssid, bss->ssid, bss->ssid_len);
-	ssid[bss->ssid_len] = '\0';
-
 	if (!(bss->capability & IE_BSS_CAP_ESS)) {
-		l_debug("Ignoring non-ESS BSS \"%s\"", ssid);
+		l_debug("Ignoring non-ESS BSS \"%.32s\"", ssid);
 		return NULL;
 	}
 
-	memset(&info, 0, sizeof(info));
-	r = scan_bss_get_rsn_info(bss, &info);
-	if (r < 0) {
-		if (r != -ENOENT)
-			return NULL;
-
-		security = security_determine(bss->capability, NULL);
-	} else
-		security = security_determine(bss->capability, &info);
-
-	path = iwd_network_get_path(station, ssid, security);
+	if (station_parse_bss(station, bss, ssid, &security, path) < 0)
+		return NULL;
 
 	network = l_hashmap_lookup(station->networks, path);
 	if (!network) {
@@ -3540,6 +3554,12 @@ static struct station *station_create(struct netdev *netdev)
 
 	station_fill_scan_freq_subsets(station);
 
+	if (iwd_is_developer_mode())
+		l_dbus_object_add_interface(dbus,
+					netdev_get_path(station->netdev),
+					IWD_STATION_DEBUG_INTERFACE,
+					station);
+
 	return station;
 }
 
@@ -3553,6 +3573,10 @@ static void station_free(struct station *station)
 	l_dbus_object_remove_interface(dbus_get_bus(),
 					netdev_get_path(station->netdev),
 					IWD_STATION_DIAGNOSTIC_INTERFACE);
+	if (iwd_is_developer_mode())
+		l_dbus_object_remove_interface(dbus_get_bus(),
+					netdev_get_path(station->netdev),
+					IWD_STATION_DEBUG_INTERFACE);
 
 	if (station->netconfig) {
 		netconfig_destroy(station->netconfig);
@@ -3761,6 +3785,19 @@ invalid_args:
 	return dbus_error_invalid_args(message);
 }
 
+static struct network *station_find_network_from_bss(struct station *station,
+						struct scan_bss *bss)
+{
+	enum security security;
+	static char path[256];
+	char ssid[33];
+
+	if (station_parse_bss(station, bss, ssid, &security, path) < 0)
+		return NULL;
+
+	return l_hashmap_lookup(station->networks, path);
+}
+
 static void station_setup_diagnostic_interface(
 					struct l_dbus_interface *interface)
 {
@@ -3777,6 +3814,53 @@ static void station_destroy_diagnostic_interface(void *user_data)
 {
 }
 
+static struct l_dbus_message *station_force_connect_bssid(struct l_dbus *dbus,
+						struct l_dbus_message *message,
+						void *user_data)
+{
+	struct station *station = user_data;
+	struct l_queue *bss_list;
+	struct scan_bss *target;
+	struct network *network;
+	struct l_dbus_message_iter iter;
+	uint8_t *mac;
+	uint32_t mac_len;
+
+	if (!l_dbus_message_get_arguments(message, "ay", &iter))
+		goto invalid_args;
+
+	if (!l_dbus_message_iter_get_fixed_array(&iter, &mac, &mac_len))
+		goto invalid_args;
+
+	if (mac_len != 6)
+		return dbus_error_invalid_args(message);
+
+	bss_list = station_get_bss_list(station);
+
+	target = l_queue_find(bss_list, bss_match_bssid, mac);
+	if (!target)
+		return dbus_error_invalid_args(message);
+
+	network = station_find_network_from_bss(station, target);
+	if (!network)
+		return dbus_error_invalid_args(message);
+
+	l_debug("Attempting forced connection to BSS "MAC, MAC_STR(mac));
+
+	return __network_connect(network, target, message);
+
+invalid_args:
+	return dbus_error_invalid_args(message);
+}
+
+static void station_setup_debug_interface(
+					struct l_dbus_interface *interface)
+{
+	l_dbus_interface_method(interface, "ConnectBssid", 0,
+					station_force_connect_bssid, "", "ay",
+					"mac");
+}
+
 static void ap_roam_frame_event(const struct mmpdu_header *hdr,
 					const void *body, size_t body_len,
 					int rssi, void *user_data)
@@ -3848,6 +3932,12 @@ static int station_init(void)
 					station_setup_diagnostic_interface,
 					station_destroy_diagnostic_interface,
 					false);
+	if (iwd_is_developer_mode())
+		l_dbus_register_interface(dbus_get_bus(),
+					IWD_STATION_DEBUG_INTERFACE,
+					station_setup_debug_interface,
+					NULL,
+					false);
 
 	if (!l_settings_get_uint(iwd_get_config(), "General",
 					"ManagementFrameProtection",
@@ -3889,6 +3979,9 @@ static void station_exit(void)
 {
 	l_dbus_unregister_interface(dbus_get_bus(),
 					IWD_STATION_DIAGNOSTIC_INTERFACE);
+	if (iwd_is_developer_mode())
+		l_dbus_unregister_interface(dbus_get_bus(),
+					IWD_STATION_DEBUG_INTERFACE);
 	l_dbus_unregister_interface(dbus_get_bus(), IWD_STATION_INTERFACE);
 	netdev_watch_remove(netdev_watch);
 	l_queue_destroy(station_list, NULL);
-- 
2.31.1

[PATCH v3 3/4] station: move Roam() under station debug interface

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

---
 src/station.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/station.c b/src/station.c
index 9a053c85..02e61c10 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3804,10 +3804,6 @@ static void station_setup_diagnostic_interface(
 	l_dbus_interface_method(interface, "GetDiagnostics", 0,
 				station_get_diagnostics, "a{sv}", "",
 				"diagnostics");
-
-	if (iwd_is_developer_mode())
-		l_dbus_interface_method(interface, "Roam", 0,
-					station_force_roam, "", "ay", "mac");
 }
 
 static void station_destroy_diagnostic_interface(void *user_data)
@@ -3859,6 +3855,8 @@ static void station_setup_debug_interface(
 	l_dbus_interface_method(interface, "ConnectBssid", 0,
 					station_force_connect_bssid, "", "ay",
 					"mac");
+	l_dbus_interface_method(interface, "Roam", 0,
+					station_force_roam, "", "ay", "mac");
 }
 
 static void ap_roam_frame_event(const struct mmpdu_header *hdr,
-- 
2.31.1

[PATCH v3 4/4] test: update force-roam to use Debug interface

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

---
 test/force-roam | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/force-roam b/test/force-roam
index b590c84a..b45ad2e6 100755
--- a/test/force-roam
+++ b/test/force-roam
@@ -9,7 +9,7 @@ if (len(sys.argv) != 3):
 
 bus = dbus.SystemBus()
 device = dbus.Interface(bus.get_object("net.connman.iwd", sys.argv[1]),
-                                    "net.connman.iwd.StationDiagnostic")
+                                    "net.connman.iwd.StationDebug")
 
 mac = sys.argv[2].replace(':', '')
 
-- 
2.31.1

Re: [PATCH v3 1/4] netdev: move failure point out of netdev_connect_common

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

Hi James,

On 8/9/21 12:42 PM, James Prestwood wrote:
> The only point of failure in netdev_connect_common was setting
> up the handshake type. Moving this outside of netdev_connect_common
> makes the code flow much better in netdev_{connect,reassociate} as
> nothing needs to be reset upon failure.
> ---
>   src/netdev.c | 37 ++++++++++++++++++++-----------------
>   1 file changed, 20 insertions(+), 17 deletions(-)
> 

<snip>

> @@ -3534,7 +3532,7 @@ build_cmd_connect:
>   	wiphy_radio_work_insert(netdev->wiphy, &netdev->work, 1,
>   				&connect_work_ops);
>   
> -	return 0;
> +	return;

I amended to get rid of this un-needed return statement

>   }
>   
>   int netdev_connect(struct netdev *netdev, struct scan_bss *bss,

Applied, thanks.

Regards,
-Denis

Re: [PATCH v3 2/4] station: add ConnectBssid() developer method

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 7608 bytes --]

Hi James,

On 8/9/21 12:42 PM, James Prestwood wrote:
> This method will initiate a connection to a specific BSS rather
> than relying on a network based connection (which the user has
> no control over which specific BSS is selected).
> ---
>   src/station.c | 129 +++++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 111 insertions(+), 18 deletions(-)
> 
> diff --git a/src/station.c b/src/station.c
> index bc4e7539..9a053c85 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -286,6 +286,34 @@ static int bss_signal_strength_compare(const void *a, const void *b, void *user)
>   	return (bss->signal_strength > new_bss->signal_strength) ? 1 : -1;
>   }
>   
> +static int station_parse_bss(struct station *station,
> +				struct scan_bss *bss, char *ssid_out,
> +				enum security *security_out,
> +				char *path_out)

The naming could be a bit clearer? Really what you care about here is the 
security type to be used in iwd_network_get_path invocation.

> +{
> +	struct ie_rsn_info info;
> +	const char *path;
> +	int r;
> +
> +	r = scan_bss_get_rsn_info(bss, &info);
> +	if (r < 0) {
> +		if (r != -ENOENT)
> +			return r;
> +
> +		*security_out = security_determine(bss->capability, NULL);
> +	} else
> +		*security_out = security_determine(bss->capability, &info);

We repeat the above lines in a bunch of places, so it might be easier to only 
perform this part, and let the iwd_network_get_path be called elsewhere.

> +
> +	memcpy(ssid_out, bss->ssid, bss->ssid_len);
> +	ssid_out[bss->ssid_len] = '\0';
> +
> +	path = iwd_network_get_path(station, ssid_out, *security_out);
> +
> +	strcpy(path_out, path);

This would also lets us avoid this part.  The 256 byte static buffer might not 
actually be enough and use of strcpy always gets flagged by static analysis.

> +
> +	return 0;
> +}
> +
>   /*
>    * Returns the network object the BSS was added to or NULL if ignored.
>    */
> @@ -293,10 +321,8 @@ static struct network *station_add_seen_bss(struct station *station,
>   						struct scan_bss *bss)
>   {
>   	struct network *network;
> -	struct ie_rsn_info info;
> -	int r;
>   	enum security security;
> -	const char *path;
> +	char path[256];
>   	char ssid[33];
>   	uint32_t kbps100 = DIV_ROUND_CLOSEST(bss->data_rate, 100000);
>   
> @@ -315,25 +341,13 @@ static struct network *station_add_seen_bss(struct station *station,
>   		return NULL;
>   	}
>   
> -	memcpy(ssid, bss->ssid, bss->ssid_len);
> -	ssid[bss->ssid_len] = '\0';
> -

I don't know why we do this in the first place since the ssid has already been 
sanitized, but ...

>   	if (!(bss->capability & IE_BSS_CAP_ESS)) {
> -		l_debug("Ignoring non-ESS BSS \"%s\"", ssid);
> +		l_debug("Ignoring non-ESS BSS \"%.32s\"", ssid);

This debug statement would now print gibberish.

>   		return NULL;
>   	}
>   
> -	memset(&info, 0, sizeof(info));
> -	r = scan_bss_get_rsn_info(bss, &info);
> -	if (r < 0) {
> -		if (r != -ENOENT)
> -			return NULL;
> -
> -		security = security_determine(bss->capability, NULL);
> -	} else
> -		security = security_determine(bss->capability, &info);
> -
> -	path = iwd_network_get_path(station, ssid, security);
> +	if (station_parse_bss(station, bss, ssid, &security, path) < 0)
> +		return NULL;
>   
>   	network = l_hashmap_lookup(station->networks, path);
>   	if (!network) {
> @@ -3540,6 +3554,12 @@ static struct station *station_create(struct netdev *netdev)
>   
>   	station_fill_scan_freq_subsets(station);
>   
> +	if (iwd_is_developer_mode())
> +		l_dbus_object_add_interface(dbus,
> +					netdev_get_path(station->netdev),
> +					IWD_STATION_DEBUG_INTERFACE,
> +					station);
> +
>   	return station;
>   }
>   
> @@ -3553,6 +3573,10 @@ static void station_free(struct station *station)
>   	l_dbus_object_remove_interface(dbus_get_bus(),
>   					netdev_get_path(station->netdev),
>   					IWD_STATION_DIAGNOSTIC_INTERFACE);
> +	if (iwd_is_developer_mode())
> +		l_dbus_object_remove_interface(dbus_get_bus(),
> +					netdev_get_path(station->netdev),
> +					IWD_STATION_DEBUG_INTERFACE);
>   
>   	if (station->netconfig) {
>   		netconfig_destroy(station->netconfig);
> @@ -3761,6 +3785,19 @@ invalid_args:
>   	return dbus_error_invalid_args(message);
>   }
>   
> +static struct network *station_find_network_from_bss(struct station *station,
> +						struct scan_bss *bss)
> +{
> +	enum security security;
> +	static char path[256];
> +	char ssid[33];
> +
> +	if (station_parse_bss(station, bss, ssid, &security, path) < 0)
> +		return NULL;
> +
> +	return l_hashmap_lookup(station->networks, path);
> +}

also note we have station_network_find()

> +
>   static void station_setup_diagnostic_interface(
>   					struct l_dbus_interface *interface)
>   {
> @@ -3777,6 +3814,53 @@ static void station_destroy_diagnostic_interface(void *user_data)
>   {
>   }
>   
> +static struct l_dbus_message *station_force_connect_bssid(struct l_dbus *dbus,
> +						struct l_dbus_message *message,
> +						void *user_data)
> +{
> +	struct station *station = user_data;
> +	struct l_queue *bss_list;
> +	struct scan_bss *target;
> +	struct network *network;
> +	struct l_dbus_message_iter iter;
> +	uint8_t *mac;
> +	uint32_t mac_len;
> +
> +	if (!l_dbus_message_get_arguments(message, "ay", &iter))
> +		goto invalid_args;
> +
> +	if (!l_dbus_message_iter_get_fixed_array(&iter, &mac, &mac_len))
> +		goto invalid_args;
> +
> +	if (mac_len != 6)
> +		return dbus_error_invalid_args(message);
> +
> +	bss_list = station_get_bss_list(station);
> +
> +	target = l_queue_find(bss_list, bss_match_bssid, mac);
> +	if (!target)
> +		return dbus_error_invalid_args(message);
> +
> +	network = station_find_network_from_bss(station, target);

It is a bit dangerous to do this directly since the BSS might actually be 
hidden.  This is developer mode only API, but still...

> +	if (!network)
> +		return dbus_error_invalid_args(message);
> +
> +	l_debug("Attempting forced connection to BSS "MAC, MAC_STR(mac));
> +
> +	return __network_connect(network, target, message);
> +
> +invalid_args:
> +	return dbus_error_invalid_args(message);
> +}
> +
> +static void station_setup_debug_interface(
> +					struct l_dbus_interface *interface)
> +{
> +	l_dbus_interface_method(interface, "ConnectBssid", 0,
> +					station_force_connect_bssid, "", "ay",
> +					"mac");
> +}
> +
>   static void ap_roam_frame_event(const struct mmpdu_header *hdr,
>   					const void *body, size_t body_len,
>   					int rssi, void *user_data)
> @@ -3848,6 +3932,12 @@ static int station_init(void)
>   					station_setup_diagnostic_interface,
>   					station_destroy_diagnostic_interface,
>   					false);
> +	if (iwd_is_developer_mode())
> +		l_dbus_register_interface(dbus_get_bus(),
> +					IWD_STATION_DEBUG_INTERFACE,
> +					station_setup_debug_interface,
> +					NULL,
> +					false);
>   
>   	if (!l_settings_get_uint(iwd_get_config(), "General",
>   					"ManagementFrameProtection",
> @@ -3889,6 +3979,9 @@ static void station_exit(void)
>   {
>   	l_dbus_unregister_interface(dbus_get_bus(),
>   					IWD_STATION_DIAGNOSTIC_INTERFACE);
> +	if (iwd_is_developer_mode())
> +		l_dbus_unregister_interface(dbus_get_bus(),
> +					IWD_STATION_DEBUG_INTERFACE);
>   	l_dbus_unregister_interface(dbus_get_bus(), IWD_STATION_INTERFACE);
>   	netdev_watch_remove(netdev_watch);
>   	l_queue_destroy(station_list, NULL);
> 

Regards,
-Denis