| Message ID | 20231218141250.202157-1-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] knownnetworks: network: support updating known network settings | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | success | GitLint |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-incremental_build | success | Incremental Build with patches |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental Build with patches |
| prestwoj/iwd-ci-testrunner | fail | test-runner - FAIL: testDPP |
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
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 >
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
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
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
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
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;