From e384ab74c24ba329db083ed22bbf91578049896b Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Fri, 12 Aug 2022 12:32:16 +0200 Subject: [PATCH 1/4] iwd: Be extra careful not to interrupt assumed activation The IWD backend would originally use .Disconnect() on IWD dbus "Station" objects to make sure IWD is out of autoconnect or that it isn't connecting to a network that NM didn't command. Later the default became to let IWD run autoconnect so now most of the time the backend just mirrors IWD's state to NMDevice's state. Now sometimes when NMDevice still seems to have an active connection but IWD has gone through one or more state changes (which we may see after a delay due to D-Bus) and is now connected to or connecting to a different network, NMDevice would first have to go through .deactivate to mirror the fact the original connection is no longer active, and it'd use .Disconnect() which could break the new connection, so check for this situation. --- src/core/devices/wifi/nm-device-iwd.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/core/devices/wifi/nm-device-iwd.c b/src/core/devices/wifi/nm-device-iwd.c index ab37cbec9..aadfc115e 100644 --- a/src/core/devices/wifi/nm-device-iwd.c +++ b/src/core/devices/wifi/nm-device-iwd.c @@ -70,6 +70,7 @@ typedef struct { bool secrets_failed : 1; bool networks_requested : 1; bool networks_changed : 1; + bool assuming : 1; gint64 last_scan; uint32_t ap_id; guint32 rate; @@ -581,6 +582,10 @@ deactivate(NMDevice *device) if (!priv->dbus_obj) return; + /* Don't cause IWD to break the connection being assumed */ + if (priv->assuming) + return; + if (priv->dbus_station_proxy) { gs_unref_variant GVariant *value = g_dbus_proxy_get_cached_property(priv->dbus_station_proxy, "State"); @@ -2719,12 +2724,20 @@ state_changed(NMDeviceIwd *self, const char *new_state) "IWD is connecting to the wrong AP, %s activation", switch_ap ? "replacing" : "aborting"); cleanup_association_attempt(self, !switch_ap); - nm_device_state_changed(device, - NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); - if (switch_ap) - assume_connection(self, ap); + if (!switch_ap) { + nm_device_state_changed(device, + NM_DEVICE_STATE_FAILED, + NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); + return; + } + + priv->assuming = TRUE; /* Don't send Station.Disconnect() */ + nm_device_state_changed(device, + NM_DEVICE_STATE_DISCONNECTED, + NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); + priv->assuming = FALSE; + assume_connection(self, ap); return; } From f6cec3b584ed263986ca9ececf345d3a52a8c8f9 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Fri, 12 Aug 2022 12:43:31 +0200 Subject: [PATCH 2/4] iwd: Let IWD handle retries When we're set to let IWD control autoconnect, don't retry connections on NM side, set retry count to 0. --- src/core/devices/wifi/nm-device-iwd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/core/devices/wifi/nm-device-iwd.c b/src/core/devices/wifi/nm-device-iwd.c index aadfc115e..73232e341 100644 --- a/src/core/devices/wifi/nm-device-iwd.c +++ b/src/core/devices/wifi/nm-device-iwd.c @@ -2275,6 +2275,18 @@ act_stage2_config(NMDevice *device, NMDeviceStateReason *out_failure_reason) goto out_fail; } + /* With priv->iwd_autoconnect we have to let IWD handle retries for + * infrastructure networks. IWD will not necessarily retry the same + * network after a failure but it will likely go into an autoconnect + * mode and we don't want to try to override the logic. We don't need + * to reset the retry count so we set no timeout. + */ + if (priv->iwd_autoconnect) { + NMSettingsConnection *sett_conn = nm_act_request_get_settings_connection(req); + + nm_settings_connection_autoconnect_retries_set(sett_conn, 0); + } + /* With priv->iwd_autoconnect, if we're assuming a connection because * of a state change to "connecting", signal stage 2 is still running. * If "connected" or "roaming", we can go right to the IP_CONFIG state From 824f2f26db465c67339260e10a728ef6f25a4c7f Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Sat, 13 Aug 2022 02:37:18 +0200 Subject: [PATCH 3/4] iwd: Work around timing when new 802.1x connection activated Try work around the issue documented by Emil Velikov in https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1264 When we mirror an 802.1x connection to an IWD config file and there's an AP in range with matching SSID, that connection should become available for activation. In IWD terms when an 802.1x network becomes a Known Network, it can be connected to using the .Connect D-Bus method. However there's a delay between writing the IWD config file and receiving the InterfaceAdded event for the Known Network so we don't immediately find out that the network can now be used. If an NM client creates a new connection for an 802.1x AP and tries to activate it quickly enough, NMDeviceIWD will not allow it to because it won't know the network is known yet. To work around this, we save the SSIDs of 802.1x connections we recently mirrored to IWD config files, for an arbitrary 2 seconds period, and we treat them as Known Networks in that period since in theory activations should succeed. The alternative proposed in the !1264 is to drop NMDeviceIWD checks that there's a Known Network for the 802.1x connection being activated since IWD will eventually perform the same checks and IWD is the ultimate authority on whether the profile is IWD-connectable. --- src/core/devices/wifi/nm-device-iwd.c | 11 +++- src/core/devices/wifi/nm-iwd-manager.c | 82 ++++++++++++++++++++++++++ src/core/devices/wifi/nm-iwd-manager.h | 2 + 3 files changed, 92 insertions(+), 3 deletions(-) diff --git a/src/core/devices/wifi/nm-device-iwd.c b/src/core/devices/wifi/nm-device-iwd.c index 73232e341..300ec7c94 100644 --- a/src/core/devices/wifi/nm-device-iwd.c +++ b/src/core/devices/wifi/nm-device-iwd.c @@ -799,7 +799,8 @@ check_connection_compatible(NMDevice *device, NMConnection *connection, GError * * thus are Known Networks. */ if (security == NM_IWD_NETWORK_SECURITY_8021X) { - if (!is_connection_known_network(connection)) { + if (!is_connection_known_network(connection) + && !nm_iwd_manager_is_recently_mirrored(nm_iwd_manager_get(), ssid)) { nm_utils_error_set_literal(error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_INCOMPATIBLE, "802.1x connections must have IWD provisioning files"); @@ -932,7 +933,9 @@ check_connection_available(NMDevice *device, */ if (nm_wifi_connection_get_iwd_ssid_and_security(connection, NULL, &security) && security == NM_IWD_NETWORK_SECURITY_8021X) { - if (!is_ap_known_network(ap)) { + if (!is_ap_known_network(ap) + && !nm_iwd_manager_is_recently_mirrored(nm_iwd_manager_get(), + nm_setting_wireless_get_ssid(s_wifi))) { nm_utils_error_set_literal( error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_TEMPORARY, @@ -2327,7 +2330,9 @@ act_stage2_config(NMDevice *device, NMDeviceStateReason *out_failure_reason) * fail, for other combinations we will let the Connect call fail * or ask us for any missing secrets through the Agent. */ - if (nm_connection_get_setting_802_1x(connection) && !is_ap_known_network(ap)) { + if (nm_connection_get_setting_802_1x(connection) && !is_ap_known_network(ap) + && !nm_iwd_manager_is_recently_mirrored(nm_iwd_manager_get(), + nm_setting_wireless_get_ssid(s_wireless))) { _LOGI(LOGD_DEVICE | LOGD_WIFI, "Activation: (wifi) access point '%s' has 802.1x security but is not configured " "in IWD.", diff --git a/src/core/devices/wifi/nm-iwd-manager.c b/src/core/devices/wifi/nm-iwd-manager.c index 2e0d51e5d..ec111329a 100644 --- a/src/core/devices/wifi/nm-iwd-manager.c +++ b/src/core/devices/wifi/nm-iwd-manager.c @@ -46,6 +46,11 @@ typedef struct { const KnownNetworkId *id; } KnownNetworkData; +typedef struct { + GBytes *ssid; + gint64 timestamp; +} RecentlyMirroredData; + typedef struct { NMManager *manager; NMSettings *settings; @@ -62,6 +67,7 @@ typedef struct { GHashTable *p2p_devices; NMIwdWfdInfo wfd_info; guint wfd_use_count; + GSList *recently_mirrored; } NMIwdManagerPrivate; struct _NMIwdManager { @@ -353,6 +359,70 @@ register_agent(NMIwdManager *self) /*****************************************************************************/ +static void +recently_mirrored_data_free(void *data) +{ + RecentlyMirroredData *rmd = data; + + g_bytes_unref(rmd->ssid); + g_free(rmd); +} + +/* When we mirror an 802.1x connection to an IWD config file, and there's an + * AP in range with matching SSID, that connection should become available + * for activation. In IWD terms when an 802.1x network becomes a Known + * Network, it can be connected to using the .Connect D-Bus method. + * + * However there's a delay between writing the IWD config file and receiving + * the InterfaceAdded event for the Known Network so we don't immediately + * find out that the network can now be used. If an NM client creates a + * new connection for an 802.1x AP and tries to activate it immediately, + * NMDeviceIWD will not allow it to because it doesn't know the network is + * known yet. To work around this, we save the SSIDs of 802.1x connections + * we recently mirrored to IWD config files, for 2 seconds, and we treat + * them as Known Networks in that period since in theory activations should + * succeed. + */ +bool +nm_iwd_manager_is_recently_mirrored(NMIwdManager *self, const GBytes *ssid) +{ + NMIwdManagerPrivate *priv = NM_IWD_MANAGER_GET_PRIVATE(self); + gint64 now = nm_utils_get_monotonic_timestamp_nsec(); + GSList *iter; + RecentlyMirroredData *rmd; + + /* Drop entries older than 2 seconds */ + while (priv->recently_mirrored) { + rmd = priv->recently_mirrored->data; + if (now < rmd->timestamp + 2000000000) + break; + + priv->recently_mirrored = g_slist_remove(priv->recently_mirrored, rmd); + recently_mirrored_data_free(rmd); + } + + for (iter = priv->recently_mirrored; iter; iter = iter->next) { + rmd = iter->data; + if (g_bytes_equal(ssid, rmd->ssid)) + return TRUE; + } + + return FALSE; +} + +static void +save_mirrored(NMIwdManager *self, GBytes *ssid) +{ + NMIwdManagerPrivate *priv = NM_IWD_MANAGER_GET_PRIVATE(self); + RecentlyMirroredData *rmd = g_malloc(sizeof(RecentlyMirroredData)); + + rmd->ssid = g_bytes_ref(ssid); + rmd->timestamp = nm_utils_get_monotonic_timestamp_nsec(); + priv->recently_mirrored = g_slist_append(priv->recently_mirrored, rmd); +} + +/*****************************************************************************/ + static KnownNetworkId * known_network_id_new(const char *name, NMIwdNetworkSecurity security) { @@ -721,6 +791,9 @@ sett_conn_changed(NMSettingsConnection *sett_conn, "iwd: changed Wi-Fi connection %s mirrored as IWD profile %s", nm_settings_connection_get_id(sett_conn), full_path); + + if (security == NM_IWD_NETWORK_SECURITY_8021X) + save_mirrored(nm_iwd_manager_get(), ssid); } /* Look up an existing NMSettingsConnection for a network that has been @@ -1283,6 +1356,7 @@ connection_added(NMSettings *settings, NMSettingsConnection *sett_conn, gpointer gs_free_error GError *error = NULL; nm_auto_unref_keyfile GKeyFile *iwd_config = NULL; NMSettingsConnectionIntFlags flags; + NMIwdNetworkSecurity security; if (!nm_streq(nm_settings_connection_get_connection_type(sett_conn), "802-11-wireless")) return; @@ -1338,6 +1412,12 @@ connection_added(NMSettings *settings, NMSettingsConnection *sett_conn, gpointer _LOGD("New Wi-Fi connection %s mirrored as IWD profile %s", nm_settings_connection_get_id(sett_conn), full_path); + + if (nm_wifi_connection_get_iwd_ssid_and_security(conn, NULL, &security) + && security == NM_IWD_NETWORK_SECURITY_8021X) { + NMSettingWireless *s_wifi = nm_connection_get_setting_wireless(conn); + save_mirrored(nm_iwd_manager_get(), nm_setting_wireless_get_ssid(s_wifi)); + } } static gboolean @@ -1952,6 +2032,8 @@ dispose(GObject *object) g_hash_table_unref(nm_steal_pointer(&priv->p2p_devices)); + g_slist_free_full(nm_steal_pointer(&priv->recently_mirrored), recently_mirrored_data_free); + G_OBJECT_CLASS(nm_iwd_manager_parent_class)->dispose(object); } diff --git a/src/core/devices/wifi/nm-iwd-manager.h b/src/core/devices/wifi/nm-iwd-manager.h index 02cd6bba5..80a275346 100644 --- a/src/core/devices/wifi/nm-iwd-manager.h +++ b/src/core/devices/wifi/nm-iwd-manager.h @@ -63,4 +63,6 @@ gboolean nm_iwd_manager_check_wfd_info_compatible(NMIwdManager *self, const NMIw gboolean nm_iwd_manager_register_wfd(NMIwdManager *self, const NMIwdWfdInfo *wfd_info); void nm_iwd_manager_unregister_wfd(NMIwdManager *self); +bool nm_iwd_manager_is_recently_mirrored(NMIwdManager *self, const GBytes *ssid); + #endif /* __NETWORKMANAGER_IWD_MANAGER_H__ */ From e3eac0908235e20b3e96f45566cf953272c027a8 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Wed, 24 Aug 2022 20:16:56 +0200 Subject: [PATCH 4/4] iwd: nm_iwd_manager_get() once and save value Call nm_iwd_manager_get once on NMDeviceIwd creation and save in priv->manager to avoid using t very often now that we have 5 new call sites. The reasoning is explained in https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1338#note_1519684 --- src/core/devices/wifi/nm-device-iwd.c | 37 +++++++++++++++------------ 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/core/devices/wifi/nm-device-iwd.c b/src/core/devices/wifi/nm-device-iwd.c index 300ec7c94..c10afdecd 100644 --- a/src/core/devices/wifi/nm-device-iwd.c +++ b/src/core/devices/wifi/nm-device-iwd.c @@ -78,6 +78,7 @@ typedef struct { GDBusMethodInvocation *pending_agent_request; NMActiveConnection *assumed_ac; guint assumed_ac_timeout; + NMIwdManager *manager; } NMDeviceIwdPrivate; struct _NMDeviceIwd { @@ -290,6 +291,7 @@ insert_ap_from_network(NMDeviceIwd *self, gint64 last_seen_msec, int16_t signal) { + NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE(self); gs_unref_object GDBusProxy *network_proxy = NULL; nm_auto_ref_string NMRefString *bss_path = nm_ref_string_new(path); NMWifiAP *ap; @@ -300,7 +302,7 @@ insert_ap_from_network(NMDeviceIwd *self, } network_proxy = - nm_iwd_manager_get_dbus_interface(nm_iwd_manager_get(), path, NM_IWD_NETWORK_INTERFACE); + nm_iwd_manager_get_dbus_interface(priv->manager, path, NM_IWD_NETWORK_INTERFACE); ap = ap_from_network(self, network_proxy, bss_path, last_seen_msec, signal); if (!ap) @@ -678,7 +680,7 @@ deactivate_async(NMDevice *device, } static gboolean -is_connection_known_network(NMConnection *connection) +is_connection_known_network(NMIwdManager *manager, NMConnection *connection) { NMIwdNetworkSecurity security; gs_free char *ssid = NULL; @@ -686,17 +688,17 @@ is_connection_known_network(NMConnection *connection) if (!nm_wifi_connection_get_iwd_ssid_and_security(connection, &ssid, &security)) return FALSE; - return nm_iwd_manager_is_known_network(nm_iwd_manager_get(), ssid, security); + return nm_iwd_manager_is_known_network(manager, ssid, security); } static gboolean -is_ap_known_network(NMWifiAP *ap) +is_ap_known_network(NMIwdManager *manager, NMWifiAP *ap) { gs_unref_object GDBusProxy *network_proxy = NULL; gs_unref_variant GVariant *known_network = NULL; network_proxy = - nm_iwd_manager_get_dbus_interface(nm_iwd_manager_get(), + nm_iwd_manager_get_dbus_interface(manager, nm_ref_string_get_str(nm_wifi_ap_get_supplicant_path(ap)), NM_IWD_NETWORK_INTERFACE); if (!network_proxy) @@ -799,8 +801,8 @@ check_connection_compatible(NMDevice *device, NMConnection *connection, GError * * thus are Known Networks. */ if (security == NM_IWD_NETWORK_SECURITY_8021X) { - if (!is_connection_known_network(connection) - && !nm_iwd_manager_is_recently_mirrored(nm_iwd_manager_get(), ssid)) { + if (!is_connection_known_network(priv->manager, connection) + && !nm_iwd_manager_is_recently_mirrored(priv->manager, ssid)) { nm_utils_error_set_literal(error, NM_UTILS_ERROR_CONNECTION_AVAILABLE_INCOMPATIBLE, "802.1x connections must have IWD provisioning files"); @@ -933,8 +935,8 @@ check_connection_available(NMDevice *device, */ if (nm_wifi_connection_get_iwd_ssid_and_security(connection, NULL, &security) && security == NM_IWD_NETWORK_SECURITY_8021X) { - if (!is_ap_known_network(ap) - && !nm_iwd_manager_is_recently_mirrored(nm_iwd_manager_get(), + if (!is_ap_known_network(priv->manager, ap) + && !nm_iwd_manager_is_recently_mirrored(priv->manager, nm_setting_wireless_get_ssid(s_wifi))) { nm_utils_error_set_literal( error, @@ -2052,7 +2054,7 @@ assume_connection(NMDeviceIwd *self, NMWifiAP *ap) * becomes "managed" only when ACTIVATED but for IWD it's really * managed when IP_CONFIG starts. */ - sett_conn = nm_iwd_manager_get_ap_mirror_connection(nm_iwd_manager_get(), ap); + sett_conn = nm_iwd_manager_get_ap_mirror_connection(priv->manager, ap); if (!sett_conn) goto error; @@ -2225,7 +2227,8 @@ act_stage1_prepare(NMDevice *device, NMDeviceStateReason *out_failure_reason) * for a first-time connection to a hidden network. If a hidden network is * a Known Network it should still have been in the AP list. */ - if (!nm_setting_wireless_get_hidden(s_wireless) || is_connection_known_network(connection)) + if (!nm_setting_wireless_get_hidden(s_wireless) + || is_connection_known_network(priv->manager, connection)) return NM_ACT_STAGE_RETURN_FAILURE; add_new: @@ -2330,8 +2333,8 @@ act_stage2_config(NMDevice *device, NMDeviceStateReason *out_failure_reason) * fail, for other combinations we will let the Connect call fail * or ask us for any missing secrets through the Agent. */ - if (nm_connection_get_setting_802_1x(connection) && !is_ap_known_network(ap) - && !nm_iwd_manager_is_recently_mirrored(nm_iwd_manager_get(), + if (nm_connection_get_setting_802_1x(connection) && !is_ap_known_network(priv->manager, ap) + && !nm_iwd_manager_is_recently_mirrored(priv->manager, nm_setting_wireless_get_ssid(s_wireless))) { _LOGI(LOGD_DEVICE | LOGD_WIFI, "Activation: (wifi) access point '%s' has 802.1x security but is not configured " @@ -2373,7 +2376,7 @@ act_stage2_config(NMDevice *device, NMDeviceStateReason *out_failure_reason) } network_proxy = nm_iwd_manager_get_dbus_interface( - nm_iwd_manager_get(), + priv->manager, nm_ref_string_get_str(nm_wifi_ap_get_supplicant_path(ap)), NM_IWD_NETWORK_INTERFACE); if (!network_proxy) { @@ -3131,7 +3134,7 @@ nm_device_iwd_set_dbus_object(NMDeviceIwd *self, GDBusObject *object) goto error; } - adapter_proxy = nm_iwd_manager_get_dbus_interface(nm_iwd_manager_get(), + adapter_proxy = nm_iwd_manager_get_dbus_interface(priv->manager, g_variant_get_string(value, NULL), NM_IWD_WIPHY_INTERFACE); if (!adapter_proxy) { @@ -3441,7 +3444,7 @@ nm_device_iwd_init(NMDeviceIwd *self) g_signal_connect(self, "notify::" NM_DEVICE_AUTOCONNECT, G_CALLBACK(autoconnect_changed), self); /* Make sure the manager is running */ - (void) nm_iwd_manager_get(); + priv->manager = g_object_ref(nm_iwd_manager_get()); } NMDevice * @@ -3473,6 +3476,8 @@ dispose(GObject *object) G_OBJECT_CLASS(nm_device_iwd_parent_class)->dispose(object); nm_assert(c_list_is_empty(&priv->aps_lst_head)); + + g_clear_object(&priv->manager); } static void