From fd1cfa6db4aef90bf29cf503cb6bee0984c86325 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Sun, 16 Sep 2018 02:41:06 +0200 Subject: [PATCH 1/4] wifi/iwd: access Network objects through ObjectManager In two places stop using g_dbus_proxy_new_* to create whole new interface proxy objects for net.connman.iwd.Network as this will normally have a huge overhead compared to asking the ObjectManager client that we already have in NMIwdManager for those proxies. dbus-monitor shows that for each network path returned by GetOrderedNetworks () -- and we call it every 10 or 20 seconds and may get many dozens of networks back -- gdbus would do the following each time: org.freedesktop.DBus.AddMatch("member=PropertiesChanged") org.freedesktop.DBus.StartServiceByName("net.connman.iwd") org.freedesktop.DBus.GetNameOwner("net.connman.iwd") org.freedesktop.DBus.Properties.GetAll("net.connman.iwd.Network") org.freedesktop.DBus.RemoveMatch("member=PropertiesChanged") --- src/devices/wifi/nm-device-iwd.c | 40 ++++++++++--------------------- src/devices/wifi/nm-iwd-manager.c | 15 ++++++++++++ src/devices/wifi/nm-iwd-manager.h | 3 +++ 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index 32d4991679..31f8cbe0c7 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -229,7 +229,7 @@ vardict_from_network_type (const char *type) } static void -insert_ap_from_network (GHashTable *aps, GDBusProxy *proxy, const char *path, int16_t signal, uint32_t ap_id) +insert_ap_from_network (GHashTable *aps, const char *path, int16_t signal, uint32_t ap_id) { gs_unref_object GDBusProxy *network_proxy = NULL; gs_unref_variant GVariant *name_value = NULL, *type_value = NULL; @@ -239,19 +239,12 @@ insert_ap_from_network (GHashTable *aps, GDBusProxy *proxy, const char *path, in GVariant *rsn; uint8_t bssid[6]; NMWifiAP *ap; - GError *error; - network_proxy = g_dbus_proxy_new_sync (g_dbus_proxy_get_connection (proxy), - G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS, - NULL, - NM_IWD_SERVICE, - path, - NM_IWD_NETWORK_INTERFACE, - NULL, &error); - if (!network_proxy) { - g_clear_error (&error); + network_proxy = nm_iwd_manager_get_dbus_interface (nm_iwd_manager_get (), + path, + NM_IWD_NETWORK_INTERFACE); + if (!network_proxy) return; - } name_value = g_dbus_proxy_get_cached_property (network_proxy, "Name"); type_value = g_dbus_proxy_get_cached_property (network_proxy, "Type"); @@ -344,10 +337,10 @@ get_ordered_networks_cb (GObject *source, GAsyncResult *res, gpointer user_data) if (compat) { while (g_variant_iter_next (networks, "(&o&sn&s)", &path, &name, &signal, &type)) - insert_ap_from_network (new_aps, priv->dbus_station_proxy, path, signal, ap_id++); + insert_ap_from_network (new_aps, path, signal, ap_id++); } else { while (g_variant_iter_next (networks, "(&on)", &path, &signal)) - insert_ap_from_network (new_aps, priv->dbus_station_proxy, path, signal, ap_id++); + insert_ap_from_network (new_aps, path, signal, ap_id++); } g_variant_iter_free (networks); @@ -1430,7 +1423,6 @@ act_stage2_config (NMDevice *device, NMDeviceStateReason *out_failure_reason) NMActRequest *req; NMWifiAP *ap; NMConnection *connection; - GError *error = NULL; GDBusProxy *network_proxy; req = nm_device_get_act_request (device); @@ -1460,21 +1452,13 @@ act_stage2_config (NMDevice *device, NMDeviceStateReason *out_failure_reason) goto out; } - /* Locate the IWD Network object */ - network_proxy = g_dbus_proxy_new_for_bus_sync (NM_IWD_BUS_TYPE, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES | - G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS, - NULL, - NM_IWD_SERVICE, - nm_wifi_ap_get_supplicant_path (ap), - NM_IWD_NETWORK_INTERFACE, - NULL, &error); + network_proxy = nm_iwd_manager_get_dbus_interface (nm_iwd_manager_get (), + nm_wifi_ap_get_supplicant_path (ap), + NM_IWD_NETWORK_INTERFACE); if (!network_proxy) { _LOGE (LOGD_DEVICE | LOGD_WIFI, - "Activation: (wifi) could not get Network interface proxy for %s: %s", - nm_wifi_ap_get_supplicant_path (ap), - error->message); - g_clear_error (&error); + "Activation: (wifi) could not get Network interface proxy for %s", + nm_wifi_ap_get_supplicant_path (ap)); NM_SET_OUT (out_failure_reason, NM_DEVICE_STATE_REASON_SUPPLICANT_FAILED); goto out; } diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 1f28d2637c..407d0c7ab5 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -786,6 +786,21 @@ nm_iwd_manager_is_known_network (NMIwdManager *self, const char *name, return g_hash_table_contains (priv->known_networks, &kn_id); } +GDBusProxy * +nm_iwd_manager_get_dbus_interface (NMIwdManager *self, const char *path, + const char *name) +{ + NMIwdManagerPrivate *priv = NM_IWD_MANAGER_GET_PRIVATE (self); + GDBusInterface *interface; + + if (!priv->object_manager) + return NULL; + + interface = g_dbus_object_manager_get_interface (priv->object_manager, path, name); + + return interface ? G_DBUS_PROXY (interface) : NULL; +} + /*****************************************************************************/ NM_DEFINE_SINGLETON_GETTER (NMIwdManager, nm_iwd_manager_get, diff --git a/src/devices/wifi/nm-iwd-manager.h b/src/devices/wifi/nm-iwd-manager.h index cf0e7262cf..57b7007a95 100644 --- a/src/devices/wifi/nm-iwd-manager.h +++ b/src/devices/wifi/nm-iwd-manager.h @@ -57,4 +57,7 @@ NMIwdManager *nm_iwd_manager_get (void); gboolean nm_iwd_manager_is_known_network (NMIwdManager *self, const char *name, NMIwdNetworkSecurity security); +GDBusProxy *nm_iwd_manager_get_dbus_interface (NMIwdManager *self, const char *path, + const char *name); + #endif /* __NETWORKMANAGER_IWD_MANAGER_H__ */ From e3aba12d14b5514dc3f6ddc9f3e58bc6ad57e03f Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Sun, 16 Sep 2018 03:42:47 +0200 Subject: [PATCH 2/4] wifi/iwd: don't save secrets in mirror NM connections When creating the mirror 802.1x connections for IWD 802.1x profiles set the NM_SETTING_SECRET_FLAG_NOT_SAVED flag on the secrets that may at some point be requested from our agent. The saved secrets could not be used anyway because of our use of NM_SECRET_AGENT_GET_SECRETS_FLAG_REQUEST_NEW in nm_device_iwd_agent_query. But also try to respect whatever secret caching policy has been configured in the IWD profile for those secrets, IWD would be responsible for storing them if it was allowed in the profile. --- src/devices/wifi/nm-iwd-manager.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 407d0c7ab5..a3da9791ce 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -430,7 +430,16 @@ mirror_8021x_connection (NMIwdManager *self, NULL)); nm_connection_add_setting (connection, setting); - setting = NM_SETTING (g_object_new (NM_TYPE_SETTING_802_1X, NULL)); + /* "password" and "private-key-password" may be requested by the IWD agent + * from NM and IWD will implement a specific secret cache policy so by + * default respect that policy and don't save copies of those secrets in + * NM settings. The saved values can not be used anyway because of our + * use of NM_SECRET_AGENT_GET_SECRETS_FLAG_REQUEST_NEW. + */ + setting = NM_SETTING (g_object_new (NM_TYPE_SETTING_802_1X, + NM_SETTING_802_1X_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, + NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, + NULL)); nm_setting_802_1x_add_eap_method (NM_SETTING_802_1X (setting), "external"); nm_connection_add_setting (connection, setting); From 44af62eb1f868d8649fe438d476d17e5ea5bbf32 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Sun, 16 Sep 2018 17:19:52 +0200 Subject: [PATCH 3/4] wifi/iwd: actually wait for Connect() call return state_changed (called when IWD signalled device state change) was supposed to not change NM device state on connect success or failure and instead wait for the DBus Connect() method callback but it would actually still call nm_device_emit_recheck_auto_activate on failure so refactor state_changed and network_connect_cb to make sure the state change and nm_device_emit_recheck_auto_activate are only called from network_connect_cb. This fixes a race where during a switch from one network to another NM would immediately start a new activation after state_changed and network_connect_cb would then handle the Connect failure and mark the new activation as failed. --- src/devices/wifi/nm-device-iwd.c | 88 +++++++++++++++++--------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index 31f8cbe0c7..ef5ad020b6 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -1285,11 +1285,14 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) { NMDeviceIwd *self = user_data; NMDevice *device = NM_DEVICE (self); + NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); gs_free_error GError *error = NULL; NMConnection *connection; NMSettingWireless *s_wifi; GBytes *ssid; gs_free char *ssid_utf8 = NULL; + NMDeviceStateReason reason = NM_DEVICE_STATE_REASON_SUPPLICANT_FAILED; + GVariant *value; if (!_nm_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, G_VARIANT_TYPE ("()"), @@ -1304,6 +1307,12 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) "Activation: (wifi) Network.Connect failed: %s", error->message); + if (nm_utils_error_is_cancelled (error, TRUE)) + return; + + if (!NM_IN_SET (nm_device_get_state (device), NM_DEVICE_STATE_CONFIG, NM_DEVICE_STATE_NEED_AUTH)) + return; + connection = nm_device_get_applied_connection (device); if (!connection) goto failed; @@ -1311,20 +1320,17 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_DBUS_ERROR)) dbus_error = g_dbus_error_get_remote_error (error); - /* If secrets were wrong, we'd be getting a net.connman.iwd.Failed */ if (nm_streq0 (dbus_error, "net.connman.iwd.Failed")) { nm_connection_clear_secrets (connection); - nm_device_state_changed (device, NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_NO_SECRETS); - } else if ( !nm_utils_error_is_cancelled (error, TRUE) - && nm_device_is_activating (device)) - goto failed; + /* If secrets were wrong, we'd be getting a net.connman.iwd.Failed */ + reason = NM_DEVICE_STATE_REASON_NO_SECRETS; + } else if (nm_streq0 (dbus_error, "net.connman.iwd.Aborted")) { + /* If agent call was cancelled we'd be getting a net.connman.iwd.Aborted */ + reason = NM_DEVICE_STATE_REASON_NO_SECRETS; + } - /* Call Disconnect to make sure IWD's autoconnect is disabled */ - cleanup_association_attempt (self, TRUE); - - return; + goto failed; } nm_assert (nm_device_get_state (device) == NM_DEVICE_STATE_CONFIG); @@ -1351,9 +1357,17 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) return; failed: - cleanup_association_attempt (self, FALSE); - nm_device_queue_state (device, NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_SUPPLICANT_FAILED); + /* Call Disconnect to make sure IWD's autoconnect is disabled */ + cleanup_association_attempt (self, TRUE); + + nm_device_queue_state (device, NM_DEVICE_STATE_FAILED, reason); + + value = g_dbus_proxy_get_cached_property (priv->dbus_station_proxy, "State"); + if (!priv->can_connect && nm_streq0 (get_variant_state (value), "disconnected")) { + priv->can_connect = true; + nm_device_emit_recheck_auto_activate (device); + } + g_variant_unref (value); } static void @@ -1737,13 +1751,6 @@ state_changed (NMDeviceIwd *self, const char *new_state) /* Don't allow scanning while connecting, disconnecting or roaming */ priv->can_scan = NM_IN_STRSET (new_state, "connected", "disconnected"); - /* Don't allow new connection until iwd exits disconnecting */ - can_connect = NM_IN_STRSET (new_state, "disconnected"); - if (can_connect != priv->can_connect) { - priv->can_connect = can_connect; - nm_device_emit_recheck_auto_activate (device); - } - if (NM_IN_STRSET (new_state, "connecting", "connected", "roaming")) { /* If we were connecting, do nothing, the confirmation of * a connection success is handled in the Device.Connect @@ -1757,40 +1764,39 @@ state_changed (NMDeviceIwd *self, const char *new_state) _LOGW (LOGD_DEVICE | LOGD_WIFI, "Unsolicited connection success, asking IWD to disconnect"); send_disconnect (self); - - return; } else if (NM_IN_STRSET (new_state, "disconnecting", "disconnected")) { - if (!iwd_connection) - return; - /* Call Disconnect on the IWD device object to make sure it * disables its own autoconnect. - * - * Note we could instead call net.connman.iwd.KnownNetworks.ForgetNetwork - * and leave the device in autoconnect. This way if NetworkManager - * changes any settings for this connection, they'd be taken into - * account on the next connection attempt. But both methods are - * a hack, we'll perhaps need an IWD API to "connect once" without - * storing anything. */ send_disconnect (self); /* - * If IWD is still handling the Connect call, let our callback - * for the dbus method handle the failure. + * If IWD is still handling the Connect call, let our Connect + * callback for the dbus method handle the failure. The main + * reason we can't handle the failure here is because the method + * callback will have more information on the specific failure + * reason. */ if (dev_state == NM_DEVICE_STATE_CONFIG) return; - nm_device_state_changed (device, - NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); - - return; - } else if (nm_streq (new_state, "unknown")) + if (iwd_connection) + nm_device_state_changed (device, + NM_DEVICE_STATE_FAILED, + NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); + } else if (!nm_streq (new_state, "unknown")) { + _LOGE (LOGD_WIFI, "State %s unknown", new_state); return; + } - _LOGE (LOGD_WIFI, "State %s unknown", new_state); + /* Don't allow new connection until iwd exits disconnecting and no + * Connect callback is pending. + */ + can_connect = NM_IN_STRSET (new_state, "disconnected"); + if (can_connect != priv->can_connect) { + priv->can_connect = can_connect; + nm_device_emit_recheck_auto_activate (device); + } } static void From 061913c58812727062eeb282a3e1d7c37289a642 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Sun, 16 Sep 2018 17:23:49 +0200 Subject: [PATCH 4/4] wifi/iwd: don't change state in secrets callback Also make sure the secrets request callback only send a reply to IWD and the Connect method return callback executes the device state change to "disconnected". --- src/devices/wifi/nm-device-iwd.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index ef5ad020b6..3bdb1e3dca 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -1248,12 +1248,7 @@ secrets_error: g_dbus_method_invocation_return_error_literal (invocation, NM_DEVICE_ERROR, NM_DEVICE_ERROR_INVALID_CONNECTION, "NM secrets request failed"); - - nm_device_state_changed (device, - NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_NO_SECRETS); - - cleanup_association_attempt (self, TRUE); + /* Now wait for the Connect callback to update device state */ } static void