[PATCH 1/5] netdev: move flag setting in netdev_reassociate

[PATCH 1/5] netdev: move flag setting in netdev_reassociate

[James Prestwood]

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

In the real world setting the flags prior to netdev_connect_common
should never cause a problem, but using the developer Roam()
command will because it forces a roam to occur with no roam scan.
This ends up running the connect work item immediately which sets
netdev->connected=true, then netdev_reassociate ends
up setting it false again right after.

To handle both ways of roaming set the flags prior to
netdev_connect_common and if it does fail reset them back to
their original values.
---
 src/netdev.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 74eb349c..e9b4a42c 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3611,19 +3611,19 @@ int netdev_reassociate(struct netdev *netdev, struct scan_bss *target_bss,
 	old_sm = netdev->sm;
 	old_hs = netdev->handshake;
 
+	netdev->associated = false;
+	netdev->operational = false;
+	netdev->connected = false;
+	netdev->in_reassoc = true;
+
 	ret = netdev_connect_common(netdev, target_bss, orig_bss, hs, NULL, 0,
 					event_filter, cb, user_data);
 	if (ret < 0)
-		return ret;
+		goto reset_flags;
 
 	if (netdev->ap)
 		memcpy(netdev->ap->prev_bssid, orig_bss->addr, ETH_ALEN);
 
-	netdev->associated = false;
-	netdev->operational = false;
-	netdev->connected = false;
-	netdev->in_reassoc = true;
-
 	netdev_rssi_polling_update(netdev);
 
 	if (old_sm)
@@ -3633,6 +3633,14 @@ int netdev_reassociate(struct netdev *netdev, struct scan_bss *target_bss,
 		handshake_state_free(old_hs);
 
 	return 0;
+
+reset_flags:
+	netdev->associated = true;
+	netdev->operational = true;
+	netdev->connected = true;
+	netdev->in_reassoc = false;
+
+	return ret;
 }
 
 static void netdev_join_adhoc_cb(struct l_genl_msg *msg, void *user_data)
-- 
2.31.1

[PATCH 2/5] network: add __network_connect

[James Prestwood]

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

This is to support the ConnectBssid developer method which
bypasses the BSS selection logic in order to force a connection
to a specific BSS.
---
 src/network.c | 63 +++++++++++++++++++++++++++++----------------------
 src/network.h |  4 ++++
 2 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/src/network.c b/src/network.c
index 4f3855e8..e160d788 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1497,35 +1497,11 @@ error:
 	return reply;
 }
 
-static struct l_dbus_message *network_connect(struct l_dbus *dbus,
-						struct l_dbus_message *message,
-						void *user_data)
+struct l_dbus_message *__network_connect(struct network *network,
+						struct scan_bss *bss,
+						struct l_dbus_message *message)
 {
-	struct network *network = user_data;
 	struct station *station = network->station;
-	struct scan_bss *bss;
-
-	l_debug("");
-
-	if (network == station_get_connected_network(station))
-		/*
-		 * The requested network is already connected, return success.
-		 */
-		return l_dbus_message_new_method_return(message);
-
-	if (network->agent_request)
-		return dbus_error_busy(message);
-
-	/*
-	 * Select the best BSS to use at this time.  If we have to query the
-	 * agent this may not be the final choice because BSS visibility can
-	 * change while we wait for the agent.
-	 */
-	bss = network_bss_select(network, true);
-
-	/* None of the BSSes is compatible with our stack */
-	if (!bss)
-		return dbus_error_not_supported(message);
 
 	switch (network_get_security(network)) {
 	case SECURITY_PSK:
@@ -1559,6 +1535,39 @@ static struct l_dbus_message *network_connect(struct l_dbus *dbus,
 	}
 }
 
+static struct l_dbus_message *network_connect(struct l_dbus *dbus,
+						struct l_dbus_message *message,
+						void *user_data)
+{
+	struct network *network = user_data;
+	struct station *station = network->station;
+	struct scan_bss *bss;
+
+	l_debug("");
+
+	if (network == station_get_connected_network(station))
+		/*
+		 * The requested network is already connected, return success.
+		 */
+		return l_dbus_message_new_method_return(message);
+
+	if (network->agent_request)
+		return dbus_error_busy(message);
+
+	/*
+	 * Select the best BSS to use at this time.  If we have to query the
+	 * agent this may not be the final choice because BSS visibility can
+	 * change while we wait for the agent.
+	 */
+	bss = network_bss_select(network, true);
+
+	/* None of the BSSes is compatible with our stack */
+	if (!bss)
+		return dbus_error_not_supported(message);
+
+	return __network_connect(network, bss, message);
+}
+
 /*
  * Returns an error message in case an error occurs.  Otherwise this function
  * returns NULL and takes a reference to message.  Callers should unref
diff --git a/src/network.h b/src/network.h
index 2cca7ffb..0c6cb21a 100644
--- a/src/network.h
+++ b/src/network.h
@@ -92,3 +92,7 @@ struct erp_cache_entry *network_get_erp_cache(struct network *network);
 
 const struct l_queue_entry *network_bss_list_get_entries(
 						struct network *network);
+
+struct l_dbus_message *__network_connect(struct network *network,
+						struct scan_bss *bss,
+						struct l_dbus_message *message);
-- 
2.31.1

[PATCH 3/5] dbus: add StationDebug interface definition

[James Prestwood]

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

This will hold methods/properties for developers to use.
---
 src/dbus.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/dbus.h b/src/dbus.h
index b3896108..7703b37f 100644
--- a/src/dbus.h
+++ b/src/dbus.h
@@ -42,6 +42,7 @@
 #define IWD_P2P_WFD_INTERFACE "net.connman.iwd.p2p.Display"
 #define IWD_STATION_DIAGNOSTIC_INTERFACE "net.connman.iwd.StationDiagnostic"
 #define IWD_AP_DIAGNOSTIC_INTERFACE "net.connman.iwd.AccessPointDiagnostic"
+#define IWD_STATION_DEBUG_INTERFACE "net.connman.iwd.StationDebug"
 
 #define IWD_BASE_PATH "/net/connman/iwd"
 #define IWD_AGENT_MANAGER_PATH IWD_BASE_PATH
-- 
2.31.1

[PATCH 4/5] station: set preparing_roam flag on Roam()

[James Prestwood]

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

The preparing_roam flag is expected to be set by a few roam
routines and normally this is done prior to the roam scan.
The Roam() developer option was not doing this and would
cause failed roams in some cases.
---
 src/station.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/station.c b/src/station.c
index 05a4a239..a441a63d 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3740,6 +3740,9 @@ static struct l_dbus_message *station_force_roam(struct l_dbus *dbus,
 
 	l_debug("Attempting forced roam to BSS "MAC, MAC_STR(mac));
 
+	/* The various roam routines expect this to be set from scanning */
+	station->preparing_roam = true;
+
 	station_transition_start(station, target);
 
 	return l_dbus_message_new_method_return(message);
-- 
2.31.1

[PATCH 5/5] station: add ConnectBssid() developer method

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4480 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/src/station.c b/src/station.c
index a441a63d..ca6043f4 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3530,6 +3530,11 @@ static struct station *station_create(struct netdev *netdev)
 
 	station_fill_scan_freq_subsets(station);
 
+	l_dbus_object_add_interface(dbus,
+					netdev_get_path(station->netdev),
+					IWD_STATION_DEBUG_INTERFACE,
+					station);
+
 	return station;
 }
 
@@ -3543,6 +3548,9 @@ static void station_free(struct station *station)
 	l_dbus_object_remove_interface(dbus_get_bus(),
 					netdev_get_path(station->netdev),
 					IWD_STATION_DIAGNOSTIC_INTERFACE);
+	l_dbus_object_remove_interface(dbus_get_bus(),
+					netdev_get_path(station->netdev),
+					IWD_STATION_DEBUG_INTERFACE);
 
 	if (station->netconfig) {
 		netconfig_destroy(station->netconfig);
@@ -3751,6 +3759,42 @@ invalid_args:
 	return dbus_error_invalid_args(message);
 }
 
+static struct network *station_find_network_from_bss(struct station *station,
+						struct scan_bss *bss)
+{
+	struct ie_rsn_info info;
+	int r;
+	enum security security;
+	const char *path;
+	char ssid[33];
+
+	memcpy(ssid, bss->ssid, bss->ssid_len);
+	ssid[bss->ssid_len] = '\0';
+
+	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);
+
+	return l_hashmap_lookup(station->networks, path);
+}
+
+static bool find_bss_by_addr(const void *a, const void *b)
+{
+	const struct scan_bss *bss = a;
+
+	if (!memcmp(bss->addr, b, 6))
+		return true;
+
+	return false;
+}
+
 static void station_setup_diagnostic_interface(
 					struct l_dbus_interface *interface)
 {
@@ -3767,6 +3811,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, find_bss_by_addr, 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)
@@ -3838,6 +3929,11 @@ static int station_init(void)
 					station_setup_diagnostic_interface,
 					station_destroy_diagnostic_interface,
 					false);
+	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",
@@ -3879,6 +3975,8 @@ static void station_exit(void)
 {
 	l_dbus_unregister_interface(dbus_get_bus(),
 					IWD_STATION_DIAGNOSTIC_INTERFACE);
+	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

Re: [PATCH 2/5] network: add __network_connect

[Denis Kenzior]

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

Hi James,

On 8/6/21 6:02 PM, James Prestwood wrote:
> This is to support the ConnectBssid developer method which
> bypasses the BSS selection logic in order to force a connection
> to a specific BSS.
> ---
>   src/network.c | 63 +++++++++++++++++++++++++++++----------------------
>   src/network.h |  4 ++++
>   2 files changed, 40 insertions(+), 27 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 3/5] dbus: add StationDebug interface definition

[Denis Kenzior]

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

Hi James,

On 8/6/21 6:02 PM, James Prestwood wrote:
> This will hold methods/properties for developers to use.
> ---
>   src/dbus.h | 1 +
>   1 file changed, 1 insertion(+)
> 

Applied, thanks.

Should we also move the Roam method to this interface?

Regards,
-Denis

Re: [PATCH 4/5] station: set preparing_roam flag on Roam()

[Denis Kenzior]

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

Hi James,

On 8/6/21 6:02 PM, James Prestwood wrote:
> The preparing_roam flag is expected to be set by a few roam
> routines and normally this is done prior to the roam scan.
> The Roam() developer option was not doing this and would
> cause failed roams in some cases.
> ---
>   src/station.c | 3 +++
>   1 file changed, 3 insertions(+)

Applied, thanks.

Regards,
-Denis

Re: [PATCH 5/5] station: add ConnectBssid() developer method

[Denis Kenzior]

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

Hi James,

On 8/6/21 6:02 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 98 insertions(+)
> 

<snip>

> @@ -3751,6 +3759,42 @@ invalid_args:
>   	return dbus_error_invalid_args(message);
>   }
>   
> +static struct network *station_find_network_from_bss(struct station *station,
> +						struct scan_bss *bss)
> +{
> +	struct ie_rsn_info info;
> +	int r;
> +	enum security security;
> +	const char *path;
> +	char ssid[33];
> +
> +	memcpy(ssid, bss->ssid, bss->ssid_len);
> +	ssid[bss->ssid_len] = '\0';
> +
> +	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);
> +
> +	return l_hashmap_lookup(station->networks, path);

Looks like station_add_seen_bss() does pretty much the same, so should this be a 
utility function that is used by both?

> +}
> +
> +static bool find_bss_by_addr(const void *a, const void *b)

station.c already has bss_match_bssid()

> +{
> +	const struct scan_bss *bss = a;
> +
> +	if (!memcmp(bss->addr, b, 6))
> +		return true;
> +
> +	return false;
> +}
> +
>   static void station_setup_diagnostic_interface(
>   					struct l_dbus_interface *interface)
>   {

Regards,
-Denis

Re: [PATCH 1/5] netdev: move flag setting in netdev_reassociate

[Denis Kenzior]

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

Hi James,

On 8/6/21 6:02 PM, James Prestwood wrote:
> In the real world setting the flags prior to netdev_connect_common
> should never cause a problem, but using the developer Roam()
> command will because it forces a roam to occur with no roam scan.
> This ends up running the connect work item immediately which sets
> netdev->connected=true, then netdev_reassociate ends
> up setting it false again right after.
> 
> To handle both ways of roaming set the flags prior to
> netdev_connect_common and if it does fail reset them back to
> their original values.
> ---
>   src/netdev.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/netdev.c b/src/netdev.c
> index 74eb349c..e9b4a42c 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -3611,19 +3611,19 @@ int netdev_reassociate(struct netdev *netdev, struct scan_bss *target_bss,
>   	old_sm = netdev->sm;
>   	old_hs = netdev->handshake;
>   
> +	netdev->associated = false;
> +	netdev->operational = false;
> +	netdev->connected = false;
> +	netdev->in_reassoc = true;
> +

Hmm, so why are we saving/restoring all 4 flags when it is only really connected 
that is the problem?

>   	ret = netdev_connect_common(netdev, target_bss, orig_bss, hs, NULL, 0,
>   					event_filter, cb, user_data);
>   	if (ret < 0)
> -		return ret;
> +		goto reset_flags;

I wonder if we can do something nicer?

One idea might be to call netdev_handshake_state_setup_connection_type() 
earlier, prior to netdev_connect_common as that is the only source of failure 
inside it.  We can then make netdev_connect_common() return void.

Another idea might be to set netdev->connected to false afterward only if 
netdev->ap exists and the work item hasn't started...?

Regards,
-Denis