[PATCH v2 1/4] knownnetworks: network: support updating known network settings

[PATCH v2 1/4] knownnetworks: network: support updating known network settings

[James Prestwood]

Currently if a known network is modified on disk the settings are not
reloaded by network. Only disconnecting/reconnecting to the network
would update the settings. This poses an issue to DPP since its
creating or updating a known network after configuration then trying
to connect. The connection itself works fine since the PSK/passphrase
is set to the network object directly, but any additional settings
are not updated.

To fix this add a new UPDATED known network event. This is then
handled from within network and all settings read from disk are
applied to the network object.
---
 src/knownnetworks.c |  4 ++++
 src/knownnetworks.h |  1 +
 src/network.c       | 27 ++++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 3 deletions(-)

v2:
 * Instead of bothering with individual settings we can just use the
   new l_settings object. This would still prefer agent-obtained creds
   set into the network object but also honor any new settings written
   to the profile if there is an ongoing connection. This is a much
   simpler approach (unless I'm missing something). Only questionable
   piece is what to do with the Security group. I hesitate to skip it
   because its what we have on disk, but it could mean the network
   object isn't synced with its settings. But I'm not sure this is
   any different than what we have today, if a profile is modified
   during an ongoing connection its settings (including security)
   won't get updated until a disconnect/reconnect.

 * Remove frequency sync on UPDATED event

diff --git a/src/knownnetworks.c b/src/knownnetworks.c
index d4d50a6f..04ce74ec 100644
--- a/src/knownnetworks.c
+++ b/src/knownnetworks.c
@@ -468,6 +468,10 @@ void known_network_update(struct network_info *network,
 	known_network_set_autoconnect(network, new->is_autoconnectable);
 
 	memcpy(&network->config, new, sizeof(struct network_config));
+
+	WATCHLIST_NOTIFY(&known_network_watches,
+				known_networks_watch_func_t,
+				KNOWN_NETWORKS_EVENT_UPDATED, network);
 }
 
 bool known_networks_foreach(known_networks_foreach_func_t function,
diff --git a/src/knownnetworks.h b/src/knownnetworks.h
index 0a5c9e25..e8ffac0b 100644
--- a/src/knownnetworks.h
+++ b/src/knownnetworks.h
@@ -35,6 +35,7 @@ struct network_info;
 enum known_networks_event {
 	KNOWN_NETWORKS_EVENT_ADDED,
 	KNOWN_NETWORKS_EVENT_REMOVED,
+	KNOWN_NETWORKS_EVENT_UPDATED,
 };
 
 struct network_info_ops {
diff --git a/src/network.c b/src/network.c
index 3918ae08..f19482d8 100644
--- a/src/network.c
+++ b/src/network.c
@@ -2001,7 +2001,8 @@ static void network_update_hotspot(struct network *network, void *user_data)
 	match_hotspot_network(info, network);
 }
 
-static void match_known_network(struct station *station, void *user_data)
+static void match_known_network(struct station *station, void *user_data,
+				bool new)
 {
 	struct network_info *info = user_data;
 	struct network *network;
@@ -2011,7 +2012,14 @@ static void match_known_network(struct station *station, void *user_data)
 		if (!network)
 			return;
 
-		network_set_info(network, info);
+		if (new)
+			network_set_info(network, info);
+
+		if (network->settings)
+			l_settings_free(network->settings);
+
+		network->settings = network_info_open_settings(info);
+
 		return;
 	}
 
@@ -2019,17 +2027,30 @@ static void match_known_network(struct station *station, void *user_data)
 	station_network_foreach(station, network_update_hotspot, info);
 }
 
+static void add_known_network(struct station *station, void *user_data)
+{
+	match_known_network(station, (struct network_info *)user_data, true);
+}
+
+static void update_known_network(struct station *station, void *user_data)
+{
+	match_known_network(station, (struct network_info *)user_data, false);
+}
+
 static void known_networks_changed(enum known_networks_event event,
 					const struct network_info *info,
 					void *user_data)
 {
 	switch (event) {
 	case KNOWN_NETWORKS_EVENT_ADDED:
-		station_foreach(match_known_network, (void *) info);
+		station_foreach(add_known_network, (void *) info);
 
 		/* Syncs frequencies of newly known network */
 		known_network_frequency_sync((struct network_info *)info);
 		break;
+	case KNOWN_NETWORKS_EVENT_UPDATED:
+		station_foreach(update_known_network, (void *) info);
+		break;
 	case KNOWN_NETWORKS_EVENT_REMOVED:
 		station_foreach(emit_known_network_removed, (void *) info);
 		break;
-- 
2.34.1

[PATCH v2 2/4] dpp: fix extra settings not being used when connecting

[James Prestwood]

After DPP completes all settings are written and synced to disk as
well as credentials set into the network object itself. The way
network/knownnetworks worked at the time did not actually re-load
these settings before the connection attempt was made which means
that extra settings not set into the network object were not used,
i.e. Hidden/Sendhostname. The connection itself always succeeded
because the network object was given the credentials directly via
setters.

Now network and knownnetworks support updating on the directory
watch callback and ADDED/UPDATED known network events. Take advantage
of this and if the network object already exists after DPP (from a
prior scan) wait unil known networks adds/updates the network and
issue the connection after that.
---
 src/dpp.c | 124 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 93 insertions(+), 31 deletions(-)

v2:
 * Fix strerror call to use a positive error code.

diff --git a/src/dpp.c b/src/dpp.c
index 1ff4b99e..af6574fb 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
@@ -101,6 +102,7 @@ struct dpp_sm {
 	uint8_t role;
 	int refcount;
 	uint32_t station_watch;
+	uint32_t known_network_watch;
 
 	uint64_t wdev_id;
 
@@ -168,6 +170,8 @@ struct dpp_sm {
 
 	struct l_dbus_message *pending;
 
+	struct l_idle *connect_idle;
+
 	/* PKEX-specific values */
 	char *pkex_id;
 	char *pkex_key;
@@ -515,6 +519,11 @@ static void dpp_reset(struct dpp_sm *dpp)
 		dpp->pkex_scan_id = 0;
 	}
 
+	if (dpp->connect_idle) {
+		l_idle_remove(dpp->connect_idle);
+		dpp->connect_idle = NULL;
+	}
+
 	dpp->state = DPP_STATE_NOTHING;
 	dpp->new_freq = 0;
 	dpp->frame_retry = 0;
@@ -570,6 +579,8 @@ static void dpp_free(struct dpp_sm *dpp)
 	if (station)
 		station_remove_state_watch(station, dpp->station_watch);
 
+	known_networks_watch_remove(dpp->known_network_watch);
+
 	l_free(dpp);
 }
 
@@ -812,8 +823,6 @@ static void dpp_write_config(struct dpp_configuration *config,
 {
 	_auto_(l_settings_free) struct l_settings *settings = l_settings_new();
 	_auto_(l_free) char *path;
-	_auto_(l_free) uint8_t *psk = NULL;
-	size_t psk_len;
 
 	path = storage_get_network_file_path(SECURITY_PSK, config->ssid);
 
@@ -822,22 +831,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);
 
@@ -856,14 +856,39 @@ static void dpp_scan_triggered(int err, void *user_data)
 		l_error("Failed to trigger DPP scan");
 }
 
+static void dpp_start_connect(struct l_idle *idle, void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
+	struct scan_bss *bss;
+	struct network *network;
+	int ret;
+
+	network = station_network_find(station, dpp->config->ssid,
+					SECURITY_PSK);
+
+	dpp_reset(dpp);
+
+	if (!network) {
+		l_debug("Network was not found!");
+		return;
+	}
+
+	l_debug("connecting to %s from DPP", network_get_ssid(network));
+
+	bss = network_bss_select(network, true);
+	ret = network_autoconnect(network, bss);
+	if (ret < 0)
+		l_warn("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 +905,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_start_connect(NULL, dpp);
 
 	return true;
 
@@ -907,6 +921,51 @@ static void dpp_scan_destroy(void *userdata)
 	dpp_reset(dpp);
 }
 
+static void dpp_known_network_watch(enum known_networks_event event,
+					const struct network_info *info,
+					void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+
+	/*
+	 * Check the following
+	 *  - DPP is enrolling
+	 *  - DPP finished (dpp->config is set)
+	 *  - This is for the network DPP just configured
+	 *  - DPP isn't already trying to connect (e.g. if the profile was
+	 *    immediately modified after DPP synced it).
+	 *  - DPP didn't start a scan for the network.
+	 */
+	if (dpp->role != DPP_CAPABILITY_ENROLLEE)
+		return;
+	if (!dpp->config)
+		return;
+	if (strcmp(info->ssid, dpp->config->ssid))
+		return;
+	if (dpp->connect_idle)
+		return;
+	if (dpp->connect_scan_id)
+		return;
+
+	switch (event) {
+	case KNOWN_NETWORKS_EVENT_ADDED:
+	case KNOWN_NETWORKS_EVENT_UPDATED:
+		/*
+		 * network.c takes care of updating the settings for the
+		 * network. This callback just tells us to begin the connection.
+		 * We do have use an idle here because there is no strict
+		 * guarantee of ordering between known network events, e.g. DPP
+		 * could have been called into prior to network and the network
+		 * object isn't updated yet.
+		 */
+		dpp->connect_idle = l_idle_create(dpp_start_connect, dpp, NULL);
+		break;
+	case KNOWN_NETWORKS_EVENT_REMOVED:
+		l_warn("profile was removed before DPP could connect");
+		break;
+	}
+}
+
 static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 				const void *body, size_t body_len,
 				int rssi, void *user_data)
@@ -1074,10 +1133,11 @@ 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);
-	else if (station) {
+	if (network && bss) {
+		l_debug("delaying connect until settings are synced");
+		dpp->config = config;
+		return;
+	} else if (station) {
 		struct scan_parameters params = {0};
 
 		params.ssid = (void *) config->ssid;
@@ -3780,6 +3840,8 @@ static void dpp_create(struct netdev *netdev)
 
 	dpp->station_watch = station_add_state_watch(station,
 					dpp_station_state_watch, dpp, NULL);
+	dpp->known_network_watch = known_networks_watch_add(
+					dpp_known_network_watch, dpp, NULL);
 
 	l_queue_push_tail(dpp_list, dpp);
 }
-- 
2.34.1

[PATCH v2 3/4] auto-t: add DPP tests to check extra settings are applied

[James Prestwood]

In order to test that extra settings are applied prior to connecting
two tests were added for hidden networks as well as one testing if
there is already an existing profile after DPP.

The reason hidden networks were used was due to the requirement of
the "Hidden" settings in the profile. If this setting doesn't get
sync'ed to disk the connection will fail.
---
 autotests/testDPP/hw.conf         |  5 ++-
 autotests/testDPP/pkex_test.py    | 72 ++++++++++++++++++++++++++++++-
 autotests/testDPP/ssidHidden.conf |  9 ++++
 autotests/testDPP/ssidHidden.psk  |  5 +++
 4 files changed, 87 insertions(+), 4 deletions(-)
 create mode 100644 autotests/testDPP/ssidHidden.conf
 create mode 100644 autotests/testDPP/ssidHidden.psk

diff --git a/autotests/testDPP/hw.conf b/autotests/testDPP/hw.conf
index a2b1470e..85f33777 100644
--- a/autotests/testDPP/hw.conf
+++ b/autotests/testDPP/hw.conf
@@ -1,5 +1,5 @@
 [SETUP]
-num_radios=4
+num_radios=5
 start_iwd=0
 hwsim_medium=yes
 
@@ -8,6 +8,7 @@ rad0=wpas.conf
 
 [HOSTAPD]
 rad1=hostapd.conf
+rad2=ssidHidden.conf
 
 [NameSpaces]
-ns0=rad2
+ns0=rad3
diff --git a/autotests/testDPP/pkex_test.py b/autotests/testDPP/pkex_test.py
index 9e0b5dd8..5df6c47d 100644
--- a/autotests/testDPP/pkex_test.py
+++ b/autotests/testDPP/pkex_test.py
@@ -26,11 +26,11 @@ class Test(unittest.TestCase):
         self.wpas.dpp_stop_listen()
         self.wpas.dpp_configurator_remove()
 
-    def start_iwd_pkex_configurator(self, device, agent=False):
+    def start_iwd_pkex_configurator(self, device, agent=False, profile='ssidCCMP.psk'):
         self.hapd.reload()
         self.hapd.wait_for_event('AP-ENABLED')
 
-        IWD.copy_to_storage('ssidCCMP.psk')
+        IWD.copy_to_storage(profile)
         device.autoconnect = True
 
         condition = 'obj.state == DeviceState.connected'
@@ -186,6 +186,71 @@ class Test(unittest.TestCase):
 
         self.agent = None
 
+    def test_existing_network(self):
+        self.hapd.reload()
+        self.hapd.wait_for_event('AP-ENABLED')
+        IWD.copy_to_storage("existingProfile.psk", "/tmp/ns0/", "ssidCCMP.psk")
+
+        # 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.start_iwd_pkex_configurator(self.device[0])
+
+        self.device[1].dpp_pkex_enroll('secret123', identifier="test")
+        self.device[1].autoconnect = True
+
+        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_hidden_network(self):
+        self.hapd_hidden.reload()
+        self.hapd_hidden.wait_for_event('AP-ENABLED')
+        IWD.copy_to_storage("existingProfile.psk", "/tmp/ns0/", "ssidHidden.psk")
+
+        # 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.start_iwd_pkex_configurator(self.device[0], profile='ssidHidden.psk')
+
+        self.device[1].dpp_pkex_enroll('secret123', identifier="test")
+        self.device[1].autoconnect = True
+
+        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/ssidHidden.psk', 'r') as f:
+            settings = f.read()
+
+        self.assertIn("Hidden=true", settings)
+
+    def test_hidden_network(self):
+        self.hapd_hidden.reload()
+        self.hapd_hidden.wait_for_event('AP-ENABLED')
+        self.start_iwd_pkex_configurator(self.device[0], profile='ssidHidden.psk')
+
+        self.device[1].dpp_pkex_enroll('secret123', identifier="test")
+        self.device[1].autoconnect = True
+
+        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/ssidHidden.psk', 'r') as f:
+            settings = f.read()
+
+        self.assertIn("Hidden=true", settings)
+
     def setUp(self):
         ns0 = ctx.get_namespace('ns0')
         self.wpas = Wpas('wpas.conf')
@@ -197,6 +262,8 @@ class Test(unittest.TestCase):
         self.device.append(self.wd_ns0.list_devices(1)[0])
         self.hapd = HostapdCLI('hostapd.conf')
         self.hapd.disable()
+        self.hapd_hidden = HostapdCLI('ssidHidden.conf')
+        self.hapd_hidden.disable()
         self.hwsim = Hwsim()
 
         self.rule_xchg_resp = self.hwsim.rules.create()
@@ -240,6 +307,7 @@ class Test(unittest.TestCase):
         self.hapd = None
         self.rule_xchg_resp = None
         IWD.clear_storage()
+        IWD.clear_storage('/tmp/ns0')
 
     @classmethod
     def setUpClass(cls):
diff --git a/autotests/testDPP/ssidHidden.conf b/autotests/testDPP/ssidHidden.conf
new file mode 100644
index 00000000..1055fb9c
--- /dev/null
+++ b/autotests/testDPP/ssidHidden.conf
@@ -0,0 +1,9 @@
+hw_mode=g
+channel=6
+ssid=ssidHidden
+
+wpa=1
+wpa_pairwise=TKIP
+wpa_passphrase=secret123
+
+ignore_broadcast_ssid=1
diff --git a/autotests/testDPP/ssidHidden.psk b/autotests/testDPP/ssidHidden.psk
new file mode 100644
index 00000000..9917294e
--- /dev/null
+++ b/autotests/testDPP/ssidHidden.psk
@@ -0,0 +1,5 @@
+[Security]
+Passphrase=secret123
+
+[Settings]
+Hidden=true
-- 
2.34.1

[PATCH v2 4/4] auto-t: increase RAM when running with valgrind (UML)

[James Prestwood]

This was done for QEMU but not for UML. Running more than a few
tests with --valgrind will generally thrown an OOM error pretty
quick.
---
 tools/runner.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/runner.py b/tools/runner.py
index 03f44611..e50ba9c0 100644
--- a/tools/runner.py
+++ b/tools/runner.py
@@ -552,7 +552,12 @@ class UmlRunner(RunnerAbstract):
 
 		kern_log = "ignore_loglevel" if "kernel" in args.verbose else "quiet"
 
-		cmd = [args.kernel, 'rootfstype=hostfs', 'ro', 'mem=256M', 'mac80211_hwsim.radios=0',
+		if self.args.valgrind:
+			ram = 512
+		else:
+			ram = 256
+
+		cmd = [args.kernel, 'rootfstype=hostfs', 'ro', f'mem={ram}M', 'mac80211_hwsim.radios=0',
 				'time-travel=inf-cpu', 'eth0=mcast', 'eth1=mcast',
 				'%s' % kern_log, 'init=%s' % self.init]
 		cmd.extend(args.to_cmd().split(' '))
-- 
2.34.1

Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[Denis Kenzior]

Hi James,

On 12/18/23 08:12, James Prestwood wrote:
> Currently if a known network is modified on disk the settings are not
> reloaded by network. Only disconnecting/reconnecting to the network
> would update the settings. This poses an issue to DPP since its
> creating or updating a known network after configuration then trying
> to connect. The connection itself works fine since the PSK/passphrase
> is set to the network object directly, but any additional settings
> are not updated.
> 
> To fix this add a new UPDATED known network event. This is then
> handled from within network and all settings read from disk are
> applied to the network object.
> ---
>   src/knownnetworks.c |  4 ++++
>   src/knownnetworks.h |  1 +
>   src/network.c       | 27 ++++++++++++++++++++++++---
>   3 files changed, 29 insertions(+), 3 deletions(-)
> 
> v2:
>   * Instead of bothering with individual settings we can just use the
>     new l_settings object. This would still prefer agent-obtained creds
>     set into the network object but also honor any new settings written

Ehh, I'm not so sure about this.  handshake_state_set_8021x_config() does a 
shallow copy of the l_settings object.  I think eap state machine will make 
copies of the info it needs, but...

>     to the profile if there is an ongoing connection. This is a much
>     simpler approach (unless I'm missing something). Only questionable

Also, netconfig_load_settings is called very early in __station_connect_network, 
so even if you overwrite the settings object here, it is probably too late to 
take any effect.

>     piece is what to do with the Security group. I hesitate to skip it
>     because its what we have on disk, but it could mean the network
>     object isn't synced with its settings. But I'm not sure this is
>     any different than what we have today, if a profile is modified
>     during an ongoing connection its settings (including security)
>     won't get updated until a disconnect/reconnect.
> 
>   * Remove frequency sync on UPDATED event
> 

Regards,
-Denis

Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[James Prestwood]

Hi Denis,

On 12/18/23 8:31 PM, Denis Kenzior wrote:
> Hi James,
>
> On 12/18/23 08:12, James Prestwood wrote:
>> Currently if a known network is modified on disk the settings are not
>> reloaded by network. Only disconnecting/reconnecting to the network
>> would update the settings. This poses an issue to DPP since its
>> creating or updating a known network after configuration then trying
>> to connect. The connection itself works fine since the PSK/passphrase
>> is set to the network object directly, but any additional settings
>> are not updated.
>>
>> To fix this add a new UPDATED known network event. This is then
>> handled from within network and all settings read from disk are
>> applied to the network object.
>> ---
>>   src/knownnetworks.c |  4 ++++
>>   src/knownnetworks.h |  1 +
>>   src/network.c       | 27 ++++++++++++++++++++++++---
>>   3 files changed, 29 insertions(+), 3 deletions(-)
>>
>> v2:
>>   * Instead of bothering with individual settings we can just use the
>>     new l_settings object. This would still prefer agent-obtained creds
>>     set into the network object but also honor any new settings written
>
> Ehh, I'm not so sure about this. handshake_state_set_8021x_config() 
> does a shallow copy of the l_settings object.  I think eap state 
> machine will make copies of the info it needs, but...
Ok, yes this wouldn't work as-is then, but we could clone it instead.
>
>>     to the profile if there is an ongoing connection. This is a much
>>     simpler approach (unless I'm missing something). Only questionable
>
> Also, netconfig_load_settings is called very early in 
> __station_connect_network, so even if you overwrite the settings 
> object here, it is probably too late to take any effect.

For this case I'm not sure there is any difference. Keeping the existing 
settings or replacing won't change netconfig once 
__station_connect_network() is called. We would need some changes in 
station to load the settings again after the connection. I'm not sure 
trying to solve this specific case of settings changing _during_ a 
connection is really in the scope of this patch set. I'm also not really 
convinced trying to fix this race condition is all that important. Its 
not really a "race" per-se, but more of just how IWD works. The profile 
is loaded upon connecting and those settings are used. If you swap 
out/modify the profile in the middle of that I don't think anyone 
would/should expect those changes to be taken for that ongoing 
connection. That's my opinion on it anyways.

What we do need is the ability to update settings on disk if IWD is 
sitting idle or connected. Currently if network/network->settings 
already exists its not possible to update unless IWD disconnects.

You mentioned copying over all settings except the security group, but 
this wouldn't handle the case of DPP (or anything) updating the profile 
with new credentials. I think we need the l_settings object to be 100% 
synced with whats on disk (but as you said, 
network->psk/network->passphrase shouldn't be altered). This then means 
the entire settings is copied, so its much more efficient to just 
replace it. Does this make sense?

Thanks,

James

>
>>     piece is what to do with the Security group. I hesitate to skip it
>>     because its what we have on disk, but it could mean the network
>>     object isn't synced with its settings. But I'm not sure this is
>>     any different than what we have today, if a profile is modified
>>     during an ongoing connection its settings (including security)
>>     won't get updated until a disconnect/reconnect.
>>
>>   * Remove frequency sync on UPDATED event
>>
>
> Regards,
> -Denis
>

Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[Denis Kenzior]

Hi James,

>>
>> Ehh, I'm not so sure about this. handshake_state_set_8021x_config() does a 
>> shallow copy of the l_settings object.  I think eap state machine will make 
>> copies of the info it needs, but...
> Ok, yes this wouldn't work as-is then, but we could clone it instead.
>>
>>>     to the profile if there is an ongoing connection. This is a much
>>>     simpler approach (unless I'm missing something). Only questionable
>>
>> Also, netconfig_load_settings is called very early in 
>> __station_connect_network, so even if you overwrite the settings object here, 
>> it is probably too late to take any effect.
> 
> For this case I'm not sure there is any difference. Keeping the existing 
> settings or replacing won't change netconfig once __station_connect_network() is 
> called. We would need some changes in station to load the settings again after 
> the connection. I'm not sure trying to solve this specific case of settings 

I guess I'm then confused on what you're trying to accomplish with this patch. 
I thought you were trying to take care of the inherent race between:
	1. Connect
	2. Write provisioning file

The result of that race is that any changes to the provisioning file are not 
taken into consideration when connect proceeds.  In particular, any networking 
related settings are ignored.  Note that today we have a bunch of logic with 
several paths for syncing to disk that take care that the settings are not 
actually lost (well, best effort anyway) when the connection is successfully 
established:
	- Special logic to update Autoconnect inside knownnetwork.c
	- Special logic for updating just the PSK bits in network_sync_settings()

I assumed that by solving the above race, you'd be solving the problem you're 
having with DPP?

> changing _during_ a connection is really in the scope of this patch set. I'm 

Okay, then this patch in particular is not needed?

> also not really convinced trying to fix this race condition is all that 
> important. Its not really a "race" per-se, but more of just how IWD works. The 
> profile is loaded upon connecting and those settings are used. If you swap 
> out/modify the profile in the middle of that I don't think anyone would/should 
> expect those changes to be taken for that ongoing connection. That's my opinion 
> on it anyways.

That's fine as well.  In that case you just need to have DPP wait until 
knownnetworks adds/updates its state.

> 
> What we do need is the ability to update settings on disk if IWD is sitting idle 
> or connected. Currently if network/network->settings already exists its not 
> possible to update unless IWD disconnects.

I'm not following.

> 
> You mentioned copying over all settings except the security group, but this 
> wouldn't handle the case of DPP (or anything) updating the profile with new 
> credentials. I think we need the l_settings object to be 100% synced with whats 
> on disk (but as you said, network->psk/network->passphrase shouldn't be 
> altered). This then means the entire settings is copied, so its much more 
> efficient to just replace it. Does this make sense?

If the settings are opened, it is too late to alter anything related to security 
since that is used by eapol, ft, netdev, etc.  You might have a chance to apply 
/ re-apply the settings used by netconfig.

Regards,
-Denis

Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[James Prestwood]

Hi Denis,

On 12/19/23 8:31 AM, Denis Kenzior wrote:
> Hi James,
>
>>>
>>> Ehh, I'm not so sure about this. handshake_state_set_8021x_config() 
>>> does a shallow copy of the l_settings object.  I think eap state 
>>> machine will make copies of the info it needs, but...
>> Ok, yes this wouldn't work as-is then, but we could clone it instead.
>>>
>>>>     to the profile if there is an ongoing connection. This is a much
>>>>     simpler approach (unless I'm missing something). Only questionable
>>>
>>> Also, netconfig_load_settings is called very early in 
>>> __station_connect_network, so even if you overwrite the settings 
>>> object here, it is probably too late to take any effect.
>>
>> For this case I'm not sure there is any difference. Keeping the 
>> existing settings or replacing won't change netconfig once 
>> __station_connect_network() is called. We would need some changes in 
>> station to load the settings again after the connection. I'm not sure 
>> trying to solve this specific case of settings 
>
> I guess I'm then confused on what you're trying to accomplish with 
> this patch. I thought you were trying to take care of the inherent 
> race between:
>     1. Connect
>     2. Write provisioning file

Things sort of evolved. I originally thought I could handle this race by 
just updating the settings but this is not really the case without 
additional changes to reapply the any netconfig changes after the 
connection. Yes this would also solve the DPP problem but I think its 
makes more sense to just sort out everything prior to issuing a 
connection rather than rely on the update to come in and quickly insert 
the missing config values. I'd hate to rely on this, its quite subtle.

>
> The result of that race is that any changes to the provisioning file 
> are not taken into consideration when connect proceeds.  In 
> particular, any networking related settings are ignored.  Note that 
> today we have a bunch of logic with several paths for syncing to disk 
> that take care that the settings are not actually lost (well, best 
> effort anyway) when the connection is successfully established:
>     - Special logic to update Autoconnect inside knownnetwork.c
>     - Special logic for updating just the PSK bits in 
> network_sync_settings()
>
> I assumed that by solving the above race, you'd be solving the problem 
> you're having with DPP?
It would yes, but not without re-applying the netconfig changes again.
>
>> changing _during_ a connection is really in the scope of this patch 
>> set. I'm 
>
> Okay, then this patch in particular is not needed?

Its at least partially needed, if a known network already exists prior 
to DPP and its overwritten there is no callback. So we may not need to 
actually touch network->settings at all as I thought. Just emit an 
UPDATED event when the profile changes. DPP can then call 
network_autoconnect and the settings _should_ be re-loaded unless I'm 
forgetting some instance where network->settings can persist, but I 
don't think it can.

>
>> also not really convinced trying to fix this race condition is all 
>> that important. Its not really a "race" per-se, but more of just how 
>> IWD works. The profile is loaded upon connecting and those settings 
>> are used. If you swap out/modify the profile in the middle of that I 
>> don't think anyone would/should expect those changes to be taken for 
>> that ongoing connection. That's my opinion on it anyways.
>
> That's fine as well.  In that case you just need to have DPP wait 
> until knownnetworks adds/updates its state.
>
>>
>> What we do need is the ability to update settings on disk if IWD is 
>> sitting idle or connected. Currently if network/network->settings 
>> already exists its not possible to update unless IWD disconnects.
>
> I'm not following.
>>
>> You mentioned copying over all settings except the security group, 
>> but this wouldn't handle the case of DPP (or anything) updating the 
>> profile with new credentials. I think we need the l_settings object 
>> to be 100% synced with whats on disk (but as you said, 
>> network->psk/network->passphrase shouldn't be altered). This then 
>> means the entire settings is copied, so its much more efficient to 
>> just replace it. Does this make sense?
>
> If the settings are opened, it is too late to alter anything related 
> to security since that is used by eapol, ft, netdev, etc. You might 
> have a chance to apply / re-apply the settings used by netconfig.

Yes, with additional changes. Not anything network/knownnetworks can do 
about it though is all I'm saying.


Thanks,

James

>
> Regards,
> -Denis

Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[Denis Kenzior]

Hi James,

>>
>> Okay, then this patch in particular is not needed?
> 
> Its at least partially needed, if a known network already exists prior to DPP 
> and its overwritten there is no callback. So we may not need to actually touch 
> network->settings at all as I thought. Just emit an UPDATED event when the 

Okay, then just do that.

> profile changes. DPP can then call network_autoconnect and the settings _should_ 
> be re-loaded unless I'm forgetting some instance where network->settings can 
> persist, but I don't think it can.

network->settings is only created/opened when a connection is attempted.

Regards,
-Denis

Re: [PATCH v2 1/4] knownnetworks: network: support updating known network settings

[James Prestwood]

On 12/19/23 9:46 AM, Denis Kenzior wrote:
> Hi James,
>
>>>
>>> Okay, then this patch in particular is not needed?
>>
>> Its at least partially needed, if a known network already exists 
>> prior to DPP and its overwritten there is no callback. So we may not 
>> need to actually touch network->settings at all as I thought. Just 
>> emit an UPDATED event when the 
>
> Okay, then just do that.
>
>> profile changes. DPP can then call network_autoconnect and the 
>> settings _should_ be re-loaded unless I'm forgetting some instance 
>> where network->settings can persist, but I don't think it can.
>
> network->settings is only created/opened when a connection is attempted.
Yeah, I think what caused most of my confusion was because DPP was using 
network_set_passphrase/psk which will create an empty settings object if 
the load fails (i.e. no profile). This then prevented 
network_autoconnect() from opening the settings (since it thought they 
already existed). So now the DPP change just syncs the profile and 
doesn't touch the network object which lets network_autoconnect() deal 
with loading, as it should.
>
> Regards,
> -Denis