public inbox for iwd@lists.linux.dev
 help / color / mirror / Atom feed
* [RFC 0/4] Fix extra settings not being taken post-DPP
@ 2023-12-13 17:25 James Prestwood
  2023-12-13 17:25 ` [RFC 1/4] knownnetworks: set ops on info in __network_info_init James Prestwood
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: James Prestwood @ 2023-12-13 17:25 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

The current logic in DPP syncs the settings to disk but this doesn't
actually turn an existing network object into a known network, i.e.
sets network->info, which then allows settings to be loaded from
disk via the open() op. Currently a connection can be made (since the
PSK is set into the network object) but any additional settings like
Hidden/SendHostname are not used.

This set is one way we could solve it, though its a bit intrusive
and requires DPP have deeper knowledge that it really should
regarding known networks. It basically forces the creation of a
known network, and I did have to modify __network_info_init to set
the default ops structure, which was a bit nasty. This is the most
direct route without having to worry about callbacks and/or idle
functions.

An alternative way would be to watch the known networks events from
DPP and connect only after an ADDED event. The reason I didn't go
this route was because I wasn't sure about the event ordering. We
need the watch in network.c to be called first (so the network->info
can be created), then connect from DPP. watchlist_add does append to
the tail of the queue so in theory as the above _should_ work so long
as the network watch is added prior to DPP, which is the case since
network's is added in init versus DPP. But I'd hate to put an implied
dependency on watchlist ordering if this changes later. This could
be coupled with an l_idle to avoid the watchlist ordering, but thats
yet another callback.

The scan cancelation patch was thrown in there as well. Since we route
through network_autoconnect/__station_connect_network after DPP there
is no canceling logic and this leads to scans failing after DPP
connects. This doesn't "break" anything but the scans should be
canceled prior to a connection.

James Prestwood (4):
  knownnetworks: set ops on info in __network_info_init
  station: unify scan cancelation
  dpp: fix non-scan connect path in DPP
  auto-t: add a few more DPP tests for seen/known networks

 autotests/testDPP/pkex_test.py | 40 ++++++++++++++
 src/dpp.c                      | 89 +++++++++++++++++++++----------
 src/knownnetworks.c            | 32 ++++++------
 src/station.c                  | 96 +++++++++++++++-------------------
 4 files changed, 162 insertions(+), 95 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC 1/4] knownnetworks: set ops on info in __network_info_init
  2023-12-13 17:25 [RFC 0/4] Fix extra settings not being taken post-DPP James Prestwood
@ 2023-12-13 17:25 ` James Prestwood
  2023-12-13 17:25 ` [RFC 2/4] station: unify scan cancelation James Prestwood
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: James Prestwood @ 2023-12-13 17:25 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

This function was only used by hotspot as a backdoor to initializing
a network info object. For the hotspot case it has its own ops
structure which is set after calling.

DPP now will need a way to create/update a known network from a
receieved configuration but it only needs the default ops structure
from known networks. Set this automatically within this function.
This won't pose an issue to hotspot as it will just overwrite the
ops pointer to its own implementation.
---
 src/knownnetworks.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/knownnetworks.c b/src/knownnetworks.c
index e2d0d515..b8f13cbc 100644
--- a/src/knownnetworks.c
+++ b/src/knownnetworks.c
@@ -125,21 +125,6 @@ void __network_config_parse(const struct l_settings *settings,
 	}
 }
 
-void __network_info_init(struct network_info *info,
-				const char *ssid, enum security security,
-				struct network_config *config)
-{
-	if (ssid)
-		strcpy(info->ssid, ssid);
-
-	info->type = security;
-
-	memcpy(&info->config, config, sizeof(struct network_config));
-
-	if (info->config.is_hidden)
-		num_known_hidden_networks++;
-}
-
 static void network_info_free(void *data)
 {
 	struct network_info *network = data;
@@ -815,6 +800,23 @@ void known_networks_add(struct network_info *network)
 				KNOWN_NETWORKS_EVENT_ADDED, network);
 }
 
+void __network_info_init(struct network_info *info,
+				const char *ssid, enum security security,
+				struct network_config *config)
+{
+	if (ssid)
+		strcpy(info->ssid, ssid);
+
+	info->type = security;
+
+	memcpy(&info->config, config, sizeof(struct network_config));
+
+	if (info->config.is_hidden)
+		num_known_hidden_networks++;
+
+	info->ops = &known_network_ops;
+}
+
 static void known_network_new(const char *ssid, enum security security,
 					struct network_config *config)
 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC 2/4] station: unify scan cancelation
  2023-12-13 17:25 [RFC 0/4] Fix extra settings not being taken post-DPP James Prestwood
  2023-12-13 17:25 ` [RFC 1/4] knownnetworks: set ops on info in __network_info_init James Prestwood
@ 2023-12-13 17:25 ` James Prestwood
  2023-12-13 17:32   ` James Prestwood
  2023-12-13 17:25 ` [RFC 3/4] dpp: fix non-scan connect path in DPP James Prestwood
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: James Prestwood @ 2023-12-13 17:25 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

The scan cancelation was all being done manually so group all the
automatic-type scans into a single cancel function. This includes
hidden, quick, owe, and periodic scans.

This is being done in order to avoid scan failures after DPP
initiates a connection. This is ultimately done through
__station_connect_network which has no cancelation logic. This
patch will move the scan cancelation into station_enter_state
when the state changes to CONNECTING. All the mentioned scans will
be canceled at that point.

DBus scans were left out intentionally as they are an explicit
type of scan rather than one that IWD starts on its own.
---
 src/station.c | 96 +++++++++++++++++++++++----------------------------
 1 file changed, 43 insertions(+), 53 deletions(-)

diff --git a/src/station.c b/src/station.c
index 10860808..95ffee05 100644
--- a/src/station.c
+++ b/src/station.c
@@ -279,16 +279,8 @@ static int station_autoconnect_next(struct station *station)
 			bss->signal_strength);
 
 		r = network_autoconnect(network, bss);
-		if (!r) {
-			if (station->quick_scan_id) {
-				scan_cancel(netdev_get_wdev_id(station->netdev),
-						station->quick_scan_id);
-				station->quick_scan_id = 0;
-				station_property_set_scanning(station, false);
-			}
-
+		if (!r)
 			return 0;
-		}
 
 		l_debug("autoconnect: network_autoconnect: %s (%d)",
 							strerror(-r), r);
@@ -1542,6 +1534,36 @@ static void station_set_drop_unicast_l2_multicast(struct station *station,
 
 static void station_signal_agent_notify(struct station *station);
 
+static void station_cancel_scans(struct station *station)
+{
+	if (station->hidden_network_scan_id) {
+		scan_cancel(netdev_get_wdev_id(station->netdev),
+				station->hidden_network_scan_id);
+
+		dbus_pending_reply(&station->hidden_pending,
+				dbus_error_failed(station->hidden_pending));
+	}
+
+	if (station->quick_scan_id) {
+		scan_cancel(netdev_get_wdev_id(station->netdev),
+				station->quick_scan_id);
+		station->quick_scan_id = 0;
+		station_property_set_scanning(station, false);
+	}
+
+	if (station->owe_hidden_scan_ids) {
+		void *ptr;
+
+		while ((ptr = l_queue_pop_head(station->owe_hidden_scan_ids)))
+			scan_cancel(netdev_get_wdev_id(station->netdev),
+					L_PTR_TO_UINT(ptr));
+
+		l_queue_destroy(station->owe_hidden_scan_ids, NULL);
+	}
+
+	periodic_scan_stop(station);
+}
+
 static void station_enter_state(struct station *station,
 						enum station_state state)
 {
@@ -1594,7 +1616,9 @@ static void station_enter_state(struct station *station,
 		if (station->signal_agent)
 			station_signal_agent_notify(station);
 
-		periodic_scan_stop(station);
+		/* Scans issued during a connection will fail */
+		station_cancel_scans(station);
+
 		break;
 	case STATION_STATE_CONNECTED:
 		l_dbus_object_add_interface(dbus,
@@ -3539,27 +3563,15 @@ void station_connect_network(struct station *station, struct network *network,
 	struct l_dbus *dbus = dbus_get_bus();
 	int err;
 
-	/*
-	 * If a hidden scan is not completed, station_is_busy would not
-	 * indicate anything is going on so we need to cancel the scan and
-	 * fail the connection now.
-	 */
-	if (station->hidden_network_scan_id) {
-		scan_cancel(netdev_get_wdev_id(station->netdev),
-				station->hidden_network_scan_id);
-
-		dbus_pending_reply(&station->hidden_pending,
-				dbus_error_failed(station->hidden_pending));
-	}
-
-	if (station->quick_scan_id) {
-		scan_cancel(netdev_get_wdev_id(station->netdev),
-				station->quick_scan_id);
-		station->quick_scan_id = 0;
-		station_property_set_scanning(station, false);
-	}
-
 	if (station_is_busy(station)) {
+		/*
+		 * All scans will be canceled when entering into a connecting
+		 * state but since we have an async disconnect callback cancel
+		 * them early so we don't potentially trigger a scan in the
+		 * middle of connecting. This also takes care of failing any
+		 * hidden network connection
+		 */
+		station_cancel_scans(station);
 		station_disconnect_onconnect(station, network, bss, message);
 
 		return;
@@ -4539,8 +4551,6 @@ static void station_free(struct station *station)
 		station->netconfig = NULL;
 	}
 
-	periodic_scan_stop(station);
-
 	if (station->signal_agent) {
 		station_signal_agent_release(station->signal_agent,
 					netdev_get_path(station->netdev));
@@ -4551,10 +4561,6 @@ static void station_free(struct station *station)
 		dbus_pending_reply(&station->connect_pending,
 				dbus_error_aborted(station->connect_pending));
 
-	if (station->hidden_pending)
-		dbus_pending_reply(&station->hidden_pending,
-				dbus_error_aborted(station->hidden_pending));
-
 	if (station->disconnect_pending)
 		dbus_pending_reply(&station->disconnect_pending,
 			dbus_error_aborted(station->disconnect_pending));
@@ -4567,23 +4573,7 @@ static void station_free(struct station *station)
 		scan_cancel(netdev_get_wdev_id(station->netdev),
 				station->dbus_scan_id);
 
-	if (station->quick_scan_id)
-		scan_cancel(netdev_get_wdev_id(station->netdev),
-				station->quick_scan_id);
-
-	if (station->hidden_network_scan_id)
-		scan_cancel(netdev_get_wdev_id(station->netdev),
-				station->hidden_network_scan_id);
-
-	if (station->owe_hidden_scan_ids) {
-		void *ptr;
-
-		while ((ptr = l_queue_pop_head(station->owe_hidden_scan_ids)))
-			scan_cancel(netdev_get_wdev_id(station->netdev),
-					L_PTR_TO_UINT(ptr));
-
-		l_queue_destroy(station->owe_hidden_scan_ids, NULL);
-	}
+	station_cancel_scans(station);
 
 	station_roam_state_clear(station);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC 3/4] dpp: fix non-scan connect path in DPP
  2023-12-13 17:25 [RFC 0/4] Fix extra settings not being taken post-DPP James Prestwood
  2023-12-13 17:25 ` [RFC 1/4] knownnetworks: set ops on info in __network_info_init James Prestwood
  2023-12-13 17:25 ` [RFC 2/4] station: unify scan cancelation James Prestwood
@ 2023-12-13 17:25 ` James Prestwood
  2023-12-13 17:25 ` [RFC 4/4] auto-t: add a few more DPP tests for seen/known networks James Prestwood
  2023-12-13 22:59 ` [RFC 0/4] Fix extra settings not being taken post-DPP Denis Kenzior
  4 siblings, 0 replies; 7+ messages in thread
From: James Prestwood @ 2023-12-13 17:25 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

When DPP completes it writes the network configuration to disk but
these changes aren't picked up by the network object until the
known networks dir watch. Currently the connection itself works
fine because the network object holds a copy of the passphrase/psk
but additional settings like Hidden/SendHostname are not updated
and not used for the connection through DPP.

In theory DPP could also watch for known networks events and wait
until the expected network is added but that is also fragile since
its not guaranteed that the DPP watch callback will happen _after_
the one in network (when the network_info) is set into the object.

Instead, DPP itself can handle the known network creation/update
itself. If a known network already exists update its config.
Otherwise create a new network_info object and set it into the
network object.
---
 src/dpp.c | 89 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 27 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index 1ff4b99e..3b42541d 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -53,6 +53,7 @@
 #include "src/network.h"
 #include "src/handshake.h"
 #include "src/nl80211util.h"
+#include "src/knownnetworks.h"
 
 #define DPP_FRAME_MAX_RETRIES 5
 #define DPP_FRAME_RETRY_TIMEOUT 1
@@ -814,6 +815,8 @@ static void dpp_write_config(struct dpp_configuration *config,
 	_auto_(l_free) char *path;
 	_auto_(l_free) uint8_t *psk = NULL;
 	size_t psk_len;
+	struct network_config network_config;
+	struct network_info *info;
 
 	path = storage_get_network_file_path(SECURITY_PSK, config->ssid);
 
@@ -822,22 +825,13 @@ static void dpp_write_config(struct dpp_configuration *config,
 		l_settings_remove_group(settings, "Security");
 	}
 
-	if (config->passphrase) {
+	if (config->passphrase)
 		l_settings_set_string(settings, "Security", "Passphrase",
 				config->passphrase);
-		if (network)
-			network_set_passphrase(network, config->passphrase);
-
-	} else if (config->psk) {
+	else if (config->psk)
 		l_settings_set_string(settings, "Security", "PreSharedKey",
 				config->psk);
 
-		psk = l_util_from_hexstring(config->psk, &psk_len);
-
-		if (network)
-			network_set_psk(network, psk);
-	}
-
 	if (config->send_hostname)
 		l_settings_set_bool(settings, "IPv4", "SendHostname", true);
 
@@ -847,6 +841,39 @@ static void dpp_write_config(struct dpp_configuration *config,
 	l_debug("Storing credential for '%s(%s)'", config->ssid,
 						security_to_str(SECURITY_PSK));
 	storage_network_sync(SECURITY_PSK, config->ssid, settings);
+
+	/*
+	 * The network has yet to be seen. Once a scan is issued station will
+	 * handle network creation and in turn the settings will be loaded from
+	 * disk.
+	 */
+	if (!network)
+		return;
+
+	/*
+	 * Otherwise we need to jump through some hoops in order to update an
+	 * existing network object with the new settings
+	 */
+	info = known_networks_find(config->ssid, SECURITY_PSK);
+
+	__network_config_parse(settings, path, &network_config);
+
+	if (info)
+		known_network_update(info, &network_config);
+	else {
+		info = l_new(struct network_info, 1);
+		__network_info_init(info, config->ssid, SECURITY_PSK, &network_config);
+		network_set_info(network, info);
+	}
+
+	if (config->passphrase)
+		network_set_passphrase(network, config->passphrase);
+	else if (config->psk) {
+		psk = l_util_from_hexstring(config->psk, &psk_len);
+
+		if (network)
+			network_set_psk(network, psk);
+	}
 }
 
 static void dpp_scan_triggered(int err, void *user_data)
@@ -856,14 +883,34 @@ static void dpp_scan_triggered(int err, void *user_data)
 		l_error("Failed to trigger DPP scan");
 }
 
+static void dpp_connect(struct dpp_sm *dpp, const char *ssid)
+{
+	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
+	struct scan_bss *bss;
+	struct network *network;
+	int ret;
+
+	network = station_network_find(station, ssid, SECURITY_PSK);
+
+	dpp_reset(dpp);
+
+	if (!network) {
+		l_debug("Network was not found after scanning");
+		return;
+	}
+
+	bss = network_bss_select(network, true);
+	ret = network_autoconnect(network, bss);
+	if (ret < 0)
+		l_debug("Failed to connect after DPP (%d) %s", ret, strerror(ret));
+}
+
 static bool dpp_scan_results(int err, struct l_queue *bss_list,
 				const struct scan_freq_set *freqs,
 				void *userdata)
 {
 	struct dpp_sm *dpp = userdata;
 	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
-	struct scan_bss *bss;
-	struct network *network;
 
 	if (err < 0)
 		goto reset;
@@ -880,18 +927,7 @@ static bool dpp_scan_results(int err, struct l_queue *bss_list,
 
 	station_set_scan_results(station, bss_list, freqs, false);
 
-	network = station_network_find(station, dpp->config->ssid,
-					SECURITY_PSK);
-
-	dpp_reset(dpp);
-
-	if (!network) {
-		l_debug("Network was not found after scanning");
-		return true;
-	}
-
-	bss = network_bss_select(network, true);
-	network_autoconnect(network, bss);
+	dpp_connect(dpp, dpp->config->ssid);
 
 	return true;
 
@@ -1075,8 +1111,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 	offchannel_cancel(dpp->wdev_id, dpp->offchannel_id);
 
 	if (network && bss)
-		__station_connect_network(station, network, bss,
-						STATION_STATE_CONNECTING);
+		dpp_connect(dpp, config->ssid);
 	else if (station) {
 		struct scan_parameters params = {0};
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC 4/4] auto-t: add a few more DPP tests for seen/known networks
  2023-12-13 17:25 [RFC 0/4] Fix extra settings not being taken post-DPP James Prestwood
                   ` (2 preceding siblings ...)
  2023-12-13 17:25 ` [RFC 3/4] dpp: fix non-scan connect path in DPP James Prestwood
@ 2023-12-13 17:25 ` James Prestwood
  2023-12-13 22:59 ` [RFC 0/4] Fix extra settings not being taken post-DPP Denis Kenzior
  4 siblings, 0 replies; 7+ messages in thread
From: James Prestwood @ 2023-12-13 17:25 UTC (permalink / raw)
  To: iwd; +Cc: James Prestwood

These ensure DPP works correctly when a network has already been seen
in the scan results as well as if its a known network.
---
 autotests/testDPP/pkex_test.py | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/autotests/testDPP/pkex_test.py b/autotests/testDPP/pkex_test.py
index 9e0b5dd8..07def453 100644
--- a/autotests/testDPP/pkex_test.py
+++ b/autotests/testDPP/pkex_test.py
@@ -186,6 +186,46 @@ class Test(unittest.TestCase):
 
         self.agent = None
 
+    def test_network_seen_before_dpp(self):
+        self.start_iwd_pkex_configurator(self.device[0])
+
+        # Scan first so a network object already exists
+        self.device[1].scan()
+        self.wd.wait_for_object_condition(self.device[1], 'obj.scanning == True')
+        self.wd.wait_for_object_condition(self.device[1], 'obj.scanning == False')
+
+        self.device[1].dpp_pkex_enroll('secret123', identifier="test")
+
+        condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_condition(self.device[1], condition)
+
+        # Check additional settings were carried over
+        with open('/tmp/ns0/ssidCCMP.psk', 'r') as f:
+            settings = f.read()
+
+        self.assertIn("SendHostname=true", settings)
+
+    def test_existing_known_network(self):
+        # Copy an invalid profile, make sure DPP overwrites it and connects
+        IWD.copy_to_storage("existingProfile.psk", "/tmp/ns0/", "ssidCCMP.psk")
+        self.start_iwd_pkex_configurator(self.device[0])
+
+        # Scan first so a network object exists, and its a known network
+        self.device[1].scan()
+        self.wd.wait_for_object_condition(self.device[1], 'obj.scanning == True')
+        self.wd.wait_for_object_condition(self.device[1], 'obj.scanning == False')
+
+        self.device[1].dpp_pkex_enroll('secret123', identifier="test")
+
+        condition = 'obj.state == DeviceState.connected'
+        self.wd.wait_for_object_condition(self.device[1], condition)
+
+        # Check additional settings were carried over
+        with open('/tmp/ns0/ssidCCMP.psk', 'r') as f:
+            settings = f.read()
+
+        self.assertIn("SendHostname=true", settings)
+
     def setUp(self):
         ns0 = ctx.get_namespace('ns0')
         self.wpas = Wpas('wpas.conf')
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC 2/4] station: unify scan cancelation
  2023-12-13 17:25 ` [RFC 2/4] station: unify scan cancelation James Prestwood
@ 2023-12-13 17:32   ` James Prestwood
  0 siblings, 0 replies; 7+ messages in thread
From: James Prestwood @ 2023-12-13 17:32 UTC (permalink / raw)
  To: iwd


On 12/13/23 09:25, James Prestwood wrote:
> The scan cancelation was all being done manually so group all the
> automatic-type scans into a single cancel function. This includes
> hidden, quick, owe, and periodic scans.
>
> This is being done in order to avoid scan failures after DPP
> initiates a connection. This is ultimately done through
> __station_connect_network which has no cancelation logic. This
> patch will move the scan cancelation into station_enter_state
> when the state changes to CONNECTING. All the mentioned scans will
> be canceled at that point.
>
> DBus scans were left out intentionally as they are an explicit
> type of scan rather than one that IWD starts on its own.
> ---
>   src/station.c | 96 +++++++++++++++++++++++----------------------------
>   1 file changed, 43 insertions(+), 53 deletions(-)
>
> diff --git a/src/station.c b/src/station.c
> index 10860808..95ffee05 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -279,16 +279,8 @@ static int station_autoconnect_next(struct station *station)
>   			bss->signal_strength);
>   
>   		r = network_autoconnect(network, bss);
> -		if (!r) {
> -			if (station->quick_scan_id) {
> -				scan_cancel(netdev_get_wdev_id(station->netdev),
> -						station->quick_scan_id);
> -				station->quick_scan_id = 0;
> -				station_property_set_scanning(station, false);
> -			}
> -
> +		if (!r)
>   			return 0;
> -		}
>   
>   		l_debug("autoconnect: network_autoconnect: %s (%d)",
>   							strerror(-r), r);
> @@ -1542,6 +1534,36 @@ static void station_set_drop_unicast_l2_multicast(struct station *station,
>   
>   static void station_signal_agent_notify(struct station *station);
>   
> +static void station_cancel_scans(struct station *station)
> +{
> +	if (station->hidden_network_scan_id) {
> +		scan_cancel(netdev_get_wdev_id(station->netdev),
> +				station->hidden_network_scan_id);
> +
> +		dbus_pending_reply(&station->hidden_pending,
> +				dbus_error_failed(station->hidden_pending));
> +	}
> +
> +	if (station->quick_scan_id) {
> +		scan_cancel(netdev_get_wdev_id(station->netdev),
> +				station->quick_scan_id);
> +		station->quick_scan_id = 0;
> +		station_property_set_scanning(station, false);
> +	}
> +
> +	if (station->owe_hidden_scan_ids) {
> +		void *ptr;
> +
> +		while ((ptr = l_queue_pop_head(station->owe_hidden_scan_ids)))
> +			scan_cancel(netdev_get_wdev_id(station->netdev),
> +					L_PTR_TO_UINT(ptr));
> +
> +		l_queue_destroy(station->owe_hidden_scan_ids, NULL);
Just realized I should set this to NULL here or we could reference it 
somewhere else after its freed.
> +	}
> +
> +	periodic_scan_stop(station);
> +}
> +
>   static void station_enter_state(struct station *station,
>   						enum station_state state)
>   {
> @@ -1594,7 +1616,9 @@ static void station_enter_state(struct station *station,
>   		if (station->signal_agent)
>   			station_signal_agent_notify(station);
>   
> -		periodic_scan_stop(station);
> +		/* Scans issued during a connection will fail */
> +		station_cancel_scans(station);
> +
>   		break;
>   	case STATION_STATE_CONNECTED:
>   		l_dbus_object_add_interface(dbus,
> @@ -3539,27 +3563,15 @@ void station_connect_network(struct station *station, struct network *network,
>   	struct l_dbus *dbus = dbus_get_bus();
>   	int err;
>   
> -	/*
> -	 * If a hidden scan is not completed, station_is_busy would not
> -	 * indicate anything is going on so we need to cancel the scan and
> -	 * fail the connection now.
> -	 */
> -	if (station->hidden_network_scan_id) {
> -		scan_cancel(netdev_get_wdev_id(station->netdev),
> -				station->hidden_network_scan_id);
> -
> -		dbus_pending_reply(&station->hidden_pending,
> -				dbus_error_failed(station->hidden_pending));
> -	}
> -
> -	if (station->quick_scan_id) {
> -		scan_cancel(netdev_get_wdev_id(station->netdev),
> -				station->quick_scan_id);
> -		station->quick_scan_id = 0;
> -		station_property_set_scanning(station, false);
> -	}
> -
>   	if (station_is_busy(station)) {
> +		/*
> +		 * All scans will be canceled when entering into a connecting
> +		 * state but since we have an async disconnect callback cancel
> +		 * them early so we don't potentially trigger a scan in the
> +		 * middle of connecting. This also takes care of failing any
> +		 * hidden network connection
> +		 */
> +		station_cancel_scans(station);
>   		station_disconnect_onconnect(station, network, bss, message);
>   
>   		return;
> @@ -4539,8 +4551,6 @@ static void station_free(struct station *station)
>   		station->netconfig = NULL;
>   	}
>   
> -	periodic_scan_stop(station);
> -
>   	if (station->signal_agent) {
>   		station_signal_agent_release(station->signal_agent,
>   					netdev_get_path(station->netdev));
> @@ -4551,10 +4561,6 @@ static void station_free(struct station *station)
>   		dbus_pending_reply(&station->connect_pending,
>   				dbus_error_aborted(station->connect_pending));
>   
> -	if (station->hidden_pending)
> -		dbus_pending_reply(&station->hidden_pending,
> -				dbus_error_aborted(station->hidden_pending));
> -
>   	if (station->disconnect_pending)
>   		dbus_pending_reply(&station->disconnect_pending,
>   			dbus_error_aborted(station->disconnect_pending));
> @@ -4567,23 +4573,7 @@ static void station_free(struct station *station)
>   		scan_cancel(netdev_get_wdev_id(station->netdev),
>   				station->dbus_scan_id);
>   
> -	if (station->quick_scan_id)
> -		scan_cancel(netdev_get_wdev_id(station->netdev),
> -				station->quick_scan_id);
> -
> -	if (station->hidden_network_scan_id)
> -		scan_cancel(netdev_get_wdev_id(station->netdev),
> -				station->hidden_network_scan_id);
> -
> -	if (station->owe_hidden_scan_ids) {
> -		void *ptr;
> -
> -		while ((ptr = l_queue_pop_head(station->owe_hidden_scan_ids)))
> -			scan_cancel(netdev_get_wdev_id(station->netdev),
> -					L_PTR_TO_UINT(ptr));
> -
> -		l_queue_destroy(station->owe_hidden_scan_ids, NULL);
> -	}
> +	station_cancel_scans(station);
>   
>   	station_roam_state_clear(station);
>   

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC 0/4] Fix extra settings not being taken post-DPP
  2023-12-13 17:25 [RFC 0/4] Fix extra settings not being taken post-DPP James Prestwood
                   ` (3 preceding siblings ...)
  2023-12-13 17:25 ` [RFC 4/4] auto-t: add a few more DPP tests for seen/known networks James Prestwood
@ 2023-12-13 22:59 ` Denis Kenzior
  4 siblings, 0 replies; 7+ messages in thread
From: Denis Kenzior @ 2023-12-13 22:59 UTC (permalink / raw)
  To: James Prestwood, iwd

Hi James,

On 12/13/23 11:25, James Prestwood wrote:
> The current logic in DPP syncs the settings to disk but this doesn't
> actually turn an existing network object into a known network, i.e.
> sets network->info, which then allows settings to be loaded from
> disk via the open() op. Currently a connection can be made (since the
> PSK is set into the network object) but any additional settings like
> Hidden/SendHostname are not used.

Okay.  So you can solve this in two ways:

1. Have dpp listen to KNOWN_NETWORKS_EVENT_ADDED and start the connection once 
the new network has been detected.
2. Fix match_known_network to take care of merging the new settings with the 
temporary settings obtained from the agent / set using network_set_*.

Advantage of 2 is that you'd be fixing the current race condition that exists 
today, where if the connection is started and a profile is modified while the 
connection is ongoing, the new settings are not taken into account.

> 
> This set is one way we could solve it, though its a bit intrusive
> and requires DPP have deeper knowledge that it really should
> regarding known networks. It basically forces the creation of a
> known network, and I did have to modify __network_info_init to set
> the default ops structure, which was a bit nasty. This is the most
> direct route without having to worry about callbacks and/or idle
> functions.
> 
> An alternative way would be to watch the known networks events from
> DPP and connect only after an ADDED event. The reason I didn't go
> this route was because I wasn't sure about the event ordering. We
> need the watch in network.c to be called first (so the network->info
> can be created), then connect from DPP. watchlist_add does append to

Just use l_idle

> the tail of the queue so in theory as the above _should_ work so long
> as the network watch is added prior to DPP, which is the case since
> network's is added in init versus DPP. But I'd hate to put an implied
> dependency on watchlist ordering if this changes later. This could
> be coupled with an l_idle to avoid the watchlist ordering, but thats
> yet another callback.
> 

Regards,
-Denis


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-12-13 22:59 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-13 17:25 [RFC 0/4] Fix extra settings not being taken post-DPP James Prestwood
2023-12-13 17:25 ` [RFC 1/4] knownnetworks: set ops on info in __network_info_init James Prestwood
2023-12-13 17:25 ` [RFC 2/4] station: unify scan cancelation James Prestwood
2023-12-13 17:32   ` James Prestwood
2023-12-13 17:25 ` [RFC 3/4] dpp: fix non-scan connect path in DPP James Prestwood
2023-12-13 17:25 ` [RFC 4/4] auto-t: add a few more DPP tests for seen/known networks James Prestwood
2023-12-13 22:59 ` [RFC 0/4] Fix extra settings not being taken post-DPP Denis Kenzior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox