From 309eba10780f83001f34b0fc8ea2955f5109233a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Jun 2021 09:47:38 +0200 Subject: [PATCH 01/12] std-aux/glib-aux: move NM_AUTO_PROTECT_ERRNO() to libnm-std-aux (cherry picked from commit 2b55408cc79831860a6b6585b2d4d8ac4f6cd20f) --- src/libnm-glib-aux/nm-macros-internal.h | 8 -------- src/libnm-std-aux/nm-std-aux.h | 8 ++++++++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libnm-glib-aux/nm-macros-internal.h b/src/libnm-glib-aux/nm-macros-internal.h index 39197ecf3a..f2d81e1ccf 100644 --- a/src/libnm-glib-aux/nm-macros-internal.h +++ b/src/libnm-glib-aux/nm-macros-internal.h @@ -121,14 +121,6 @@ _nm_auto_free_gstring(GString **str) } #define nm_auto_free_gstring nm_auto(_nm_auto_free_gstring) -static inline void -_nm_auto_protect_errno(const int *p_saved_errno) -{ - errno = *p_saved_errno; -} -#define NM_AUTO_PROTECT_ERRNO(errsv_saved) \ - nm_auto(_nm_auto_protect_errno) _nm_unused const int errsv_saved = (errno) - NM_AUTO_DEFINE_FCN0(GSource *, _nm_auto_unref_gsource, g_source_unref); #define nm_auto_unref_gsource nm_auto(_nm_auto_unref_gsource) diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index b153866fd8..4643adc01c 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -695,6 +695,14 @@ nm_close(int fd) NM_AUTO_DEFINE_FCN_VOID0(void *, _nm_auto_free_impl, free); #define nm_auto_free nm_auto(_nm_auto_free_impl) +static inline void +_nm_auto_protect_errno(const int *p_saved_errno) +{ + errno = *p_saved_errno; +} +#define NM_AUTO_PROTECT_ERRNO(errsv_saved) \ + nm_auto(_nm_auto_protect_errno) _nm_unused const int errsv_saved = (errno) + /*****************************************************************************/ static inline void From 29d64f148e0bec94245bc4e42cd6141736a15e99 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Jun 2021 09:48:13 +0200 Subject: [PATCH 02/12] glib-aux: use NM_AUTO_PROTECT_ERRNO() in nm_auto_close and nm_auto_fclose (cherry picked from commit f9f453994b6944005ee5e779eed34a3a32111c78) --- src/libnm-std-aux/nm-std-aux.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index 4643adc01c..17b268816d 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -709,10 +709,8 @@ static inline void _nm_auto_close(int *pfd) { if (*pfd >= 0) { - int errsv = errno; - + NM_AUTO_PROTECT_ERRNO(errsv); (void) nm_close(*pfd); - errno = errsv; } } #define nm_auto_close nm_auto(_nm_auto_close) @@ -721,10 +719,8 @@ static inline void _nm_auto_fclose(FILE **pfd) { if (*pfd) { - int errsv = errno; - + NM_AUTO_PROTECT_ERRNO(errsv); (void) fclose(*pfd); - errno = errsv; } } #define nm_auto_fclose nm_auto(_nm_auto_fclose) From 9e1d29d437f6aef68c8833f8183a67849c66d303 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Jun 2021 23:04:08 +0200 Subject: [PATCH 03/12] core: set _nm_utils_is_manager_process as first thing in daemon (cherry picked from commit 6c5070da55dd79bb9a28d55100ce72ed2a086f6a) --- src/core/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index cfcdb860dd..3cb5c07f7f 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -304,14 +304,14 @@ main(int argc, char *argv[]) const char *const * warnings; int errsv; + _nm_utils_is_manager_process = TRUE; + /* Known to cause a possible deadlock upon GDBus initialization: * https://bugzilla.gnome.org/show_bug.cgi?id=674885 */ g_type_ensure(G_TYPE_SOCKET); g_type_ensure(G_TYPE_DBUS_CONNECTION); g_type_ensure(NM_TYPE_DBUS_MANAGER); - _nm_utils_is_manager_process = TRUE; - main_loop = g_main_loop_new(NULL, FALSE); /* we determine a first-start (contrary to a restart during the same boot) From 36eacd85286c8d3af61ac09d39144b3c2fd4eb45 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Jun 2021 22:18:50 +0200 Subject: [PATCH 04/12] libnm/keyfile: ignore [wifi].seen-bssids for keyfile "seen-bssids" primarily gets stored to "/var/lib/NetworkManager/seen-bssids", it's not a regular property. We want this property to be serialized/deserialized to/from GVariant, because we expose these settings on the API like a property of the profile. But it cannot be modified via nmcli, it cannot be stored to ifcfg files, and it makes not sense to store it to keyfile either. Stop doing that. (cherry picked from commit d9ebcc8646ebd815e5b47d04aff8e9921adb7d6c) --- src/libnm-core-impl/nm-keyfile.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index 88aa0d46cd..c6c4a2eb57 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -2781,6 +2781,9 @@ static const ParseInfoSetting *const parse_infos[_NM_META_SETTING_TYPE_NUM] = { .parser = mac_address_parser_ETHER_cloned, ), PARSE_INFO_PROPERTY(NM_SETTING_WIRELESS_MAC_ADDRESS, .parser = mac_address_parser_ETHER, ), + PARSE_INFO_PROPERTY(NM_SETTING_WIRELESS_SEEN_BSSIDS, + .parser_skip = TRUE, + .writer_skip = TRUE, ), PARSE_INFO_PROPERTY(NM_SETTING_WIRELESS_SSID, .parser = ssid_parser, .writer = ssid_writer, ), ), ), From bb8481f10128d90dedbc4e02c60708e08e462896 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Jun 2021 22:37:36 +0200 Subject: [PATCH 05/12] libnm: special handle serialization to D-Bus for "wifi.seen-bssid" "wifi.seen-bssid" is an unusual property, therefore very ugly due to the inconsistency. It is not a regular user configuration that makes sense to store to disk or modify by the user. It gets populated by the daemon, and stored in "/var/lib/NetworkManager/seen-bssids" file. As such, how to convert this to/from D-Bus needs special handling. This means, that the to/from D-Bus functions will only serialize the property when the seen-bssids are specified via NMConnectionSerializationOptions, which is what the daemon does. Also, the daemon ignores seen-bssids when parsing the variant. This has the odd effect that when the client converts a setting to GVariant, the seen-bssids gets lost. That means, a conversion to GVariant and back looses information. I think that is OK in this case, because the main point of to/from D-Bus is not to have a lossless GVariant representation of a setting, but to transfer the setting via D-Bus between client and daemon. And transferring seen-bssids via D-Bus makes only sense from the daemon to the client. (cherry picked from commit 4a4f214722e29ae437f428dcb803e4b63f74f617) --- src/libnm-core-impl/nm-setting-wireless.c | 62 +++++++++++++++++++---- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wireless.c b/src/libnm-core-impl/nm-setting-wireless.c index 111116965a..5bdedf9bfb 100644 --- a/src/libnm-core-impl/nm-setting-wireless.c +++ b/src/libnm-core-impl/nm-setting-wireless.c @@ -750,19 +750,54 @@ _to_dbus_fcn_seen_bssids(const NMSettInfoSetting * sett_info, NMConnectionSerializationFlags flags, const NMConnectionSerializationOptions *options) { - NMSettingWirelessPrivate *priv; - - if (options && options->seen_bssids) { + if (options && options->seen_bssids) return options->seen_bssids[0] ? g_variant_new_strv(options->seen_bssids, -1) : NULL; + + /* The seen-bssid property is special. It cannot be converted to D-Bus + * like regular properties, only via the "options". + * + * This basically means, that only the daemon can provide seen-bssids as GVariant, + * while when a client converts the property to GVariant, it gets lost. + * + * This has the odd effect, that when the client converts the setting to GVariant + * and back, the seen-bssids gets lost. That is kinda desired here, because the to_dbus_fcn() + * and from_dbus_fcn() have the meaning of how a setting gets transferred via D-Bus, + * and not necessarily a loss-less conversion into another format and back. And when + * transferring via D-Bus, then the option makes only sense when sending it from + * the daemon to the client, not otherwise. */ + return NULL; +} + +static gboolean +_from_dbus_fcn_seen_bssids(NMSetting * setting, + GVariant * connection_dict, + const char * property, + GVariant * value, + NMSettingParseFlags parse_flags, + GError ** error) +{ + NMSettingWirelessPrivate *priv; + gs_free const char ** s = NULL; + gsize len; + gsize i; + + if (_nm_utils_is_manager_process) { + /* in the manager process, we don't accept seen-bssid from the client. + * Do nothing. */ + return TRUE; } priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - if (!priv->seen_bssids || priv->seen_bssids->len == 0) - return NULL; + nm_clear_pointer(&priv->seen_bssids, g_ptr_array_unref); - return g_variant_new_strv((const char *const *) priv->seen_bssids->pdata, - priv->seen_bssids->len); + s = g_variant_get_strv(value, &len); + if (len > 0) { + priv->seen_bssids = g_ptr_array_new_full(len, g_free); + for (i = 0; i < len; i++) + g_ptr_array_add(priv->seen_bssids, g_strdup(s[i])); + } + return TRUE; } /** @@ -1075,12 +1110,18 @@ compare_property(const NMSettInfoSetting *sett_info, NMSetting * set_b, NMSettingCompareFlags flags) { - if (nm_streq(sett_info->property_infos[property_idx].name, - NM_SETTING_WIRELESS_CLONED_MAC_ADDRESS)) { + if (sett_info->property_infos[property_idx].param_spec + == obj_properties[PROP_CLONED_MAC_ADDRESS]) { return !set_b || nm_streq0(NM_SETTING_WIRELESS_GET_PRIVATE(set_a)->cloned_mac_address, NM_SETTING_WIRELESS_GET_PRIVATE(set_b)->cloned_mac_address); } + if (sett_info->property_infos[property_idx].param_spec == obj_properties[PROP_SEEN_BSSIDS]) { + return !set_b + || (nm_strv_ptrarray_cmp(NM_SETTING_WIRELESS_GET_PRIVATE(set_a)->seen_bssids, + NM_SETTING_WIRELESS_GET_PRIVATE(set_b)->seen_bssids) + == 0); + } return NM_SETTING_CLASS(nm_setting_wireless_parent_class) ->compare_property(sett_info, property_idx, con_a, set_a, con_b, set_b, flags); @@ -1744,7 +1785,8 @@ nm_setting_wireless_class_init(NMSettingWirelessClass *klass) properties_override, obj_properties[PROP_SEEN_BSSIDS], NM_SETT_INFO_PROPERT_TYPE_DBUS(G_VARIANT_TYPE_STRING_ARRAY, - .to_dbus_fcn = _to_dbus_fcn_seen_bssids, )); + .to_dbus_fcn = _to_dbus_fcn_seen_bssids, + .from_dbus_fcn = _from_dbus_fcn_seen_bssids, )); /** * NMSettingWireless:mtu: From b31cafc4a4b82405739406db74ca6aa6e2147e51 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Jun 2021 22:28:42 +0200 Subject: [PATCH 06/12] settings: don't populate seen-bssids list from connection profile ifcfg-rh plugin never stored the seen bssid list to file, and keyfile no longer does, and it's no longer parsed from GVariant. So there is actually no way how anything could be set here. The seen-bssids should only be populate from "/var/lib/NetworkManager/seen-bssids". Nowhere else. (cherry picked from commit 15a02717813679df90ab9fade24b0036e40d47e1) --- src/core/settings/nm-settings-connection.c | 24 ---------------------- 1 file changed, 24 deletions(-) diff --git a/src/core/settings/nm-settings-connection.c b/src/core/settings/nm-settings-connection.c index 641f3297b8..de5b7c296e 100644 --- a/src/core/settings/nm-settings-connection.c +++ b/src/core/settings/nm-settings-connection.c @@ -2321,30 +2321,6 @@ _nm_settings_connection_register_kf_dbs(NMSettingsConnection *self, for (i = len; i > 0;) g_hash_table_add(priv->seen_bssids, g_steal_pointer(&tmp_strv[--i])); nm_clear_g_free(&tmp_strv); - } else { - NMSettingWireless *s_wifi; - - _LOGT("no seen-bssids from keyfile database \"%s\"", - nm_key_file_db_get_filename(priv->kf_db_seen_bssids)); - - /* If this connection didn't have an entry in the seen-bssids database, - * maybe this is the first time we've read it in, so populate the - * seen-bssids list from the deprecated seen-bssids property of the - * wifi setting. - */ - s_wifi = - nm_connection_get_setting_wireless(nm_settings_connection_get_connection(self)); - if (s_wifi) { - len = nm_setting_wireless_get_num_seen_bssids(s_wifi); - if (len > 0) { - priv->seen_bssids = _seen_bssids_hash_new(); - for (i = 0; i < len; i++) { - const char *bssid = nm_setting_wireless_get_seen_bssid(s_wifi, i); - - g_hash_table_add(priv->seen_bssids, g_strdup(bssid)); - } - } - } } } } From 1afc327c5d5deabc0239a5d44ae86d55ab8355b2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Jun 2021 16:03:29 +0200 Subject: [PATCH 07/12] settings: limit number of seen-bssids and preserve order Previously, there was no limit how many seen-bssids are tracked. That seems problematic, also because there is no API how to get rid of an excessive list of entries. We should limit the number of entries. Add an (arbitrary) limit of 30. But this means that we drop the surplus of entries, and for that it seems important to keep the newest, most recently seen entries. Previously, entries were merely sorted ASCIIbetically. Now, honor their order (with most recently seen first). Also, normalize the BSSIDs. From internal code, we should only get normalize strings, but when we load them from disk, they might be bogus. As we might cut of the list, we don't want that invalid entries cut of valid ones. And of course, invalid entries make no sense at all. (cherry picked from commit 8278719840fe926bf103cd47547ecc38fc882477) --- src/core/settings/nm-settings-connection.c | 209 +++++++++++++++------ src/core/settings/nm-settings-connection.h | 2 - 2 files changed, 155 insertions(+), 56 deletions(-) diff --git a/src/core/settings/nm-settings-connection.c b/src/core/settings/nm-settings-connection.c index de5b7c296e..36ef6acb2f 100644 --- a/src/core/settings/nm-settings-connection.c +++ b/src/core/settings/nm-settings-connection.c @@ -11,6 +11,7 @@ #include "c-list/src/c-list.h" #include "libnm-glib-aux/nm-keyfile-aux.h" +#include "libnm-glib-aux/nm-c-list.h" #include "libnm-core-aux-intern/nm-common-macros.h" #include "nm-config.h" #include "nm-config-data.h" @@ -30,6 +31,8 @@ #define AUTOCONNECT_RETRIES_FOREVER -1 #define AUTOCONNECT_RESET_RETRIES_TIMER 300 +#define SEEN_BSSIDS_MAX 30 + #define _NM_SETTINGS_UPDATE2_FLAG_ALL_PERSIST_MODES \ ((NMSettingsUpdate2Flags) (NM_SETTINGS_UPDATE2_FLAG_TO_DISK \ | NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY \ @@ -59,6 +62,56 @@ nm_settings_connections_array_to_connections(NMSettingsConnection *const *connec /*****************************************************************************/ +typedef struct { + char bssid[sizeof(NMEtherAddr) * 3]; + CList seen_bssids_lst; +} SeenBssidEntry; + +static inline SeenBssidEntry * +_seen_bssid_entry_init_stale(SeenBssidEntry *entry, const NMEtherAddr *bssid_bin) +{ + _nm_utils_hwaddr_ntoa(bssid_bin, sizeof(NMEtherAddr), TRUE, entry->bssid, sizeof(entry->bssid)); + return entry; +} + +static inline SeenBssidEntry * +_seen_bssid_entry_new_stale_bin(const NMEtherAddr *bssid_bin) +{ + return _seen_bssid_entry_init_stale(g_slice_new(SeenBssidEntry), bssid_bin); +} + +static inline SeenBssidEntry * +_seen_bssid_entry_new_stale_copy(const SeenBssidEntry *src) +{ + SeenBssidEntry *entry; + + entry = g_slice_new(SeenBssidEntry); + memcpy(entry->bssid, src->bssid, sizeof(entry->bssid)); + return entry; +} + +static void +_seen_bssid_entry_free(gpointer data) +{ + SeenBssidEntry *entry = data; + + c_list_unlink_stale(&entry->seen_bssids_lst); + nm_g_slice_free(entry); +} + +/*****************************************************************************/ + +static GHashTable * +_seen_bssids_hash_new(void) +{ + return g_hash_table_new_full(nm_str_hash, + g_str_equal, + (GDestroyNotify) _seen_bssid_entry_free, + NULL); +} + +/*****************************************************************************/ + NM_GOBJECT_PROPERTIES_DEFINE(NMSettingsConnection, PROP_UNSAVED, PROP_FLAGS, PROP_FILENAME, ); enum { UPDATED_INTERNAL, FLAGS_CHANGED, LAST_SIGNAL }; @@ -99,7 +152,8 @@ typedef struct _NMSettingsConnectionPrivate { */ GVariant *agent_secrets; - GHashTable *seen_bssids; /* Up-to-date BSSIDs that's been seen for the connection */ + CList seen_bssids_lst_head; + GHashTable *seen_bssids_hash; guint64 timestamp; /* Up-to-date timestamp of connection use */ @@ -167,7 +221,9 @@ static const GDBusSignalInfo signal_info_updated; static const GDBusSignalInfo signal_info_removed; static const NMDBusInterfaceInfoExtended interface_info_settings_connection; -static void update_agent_secrets_cache(NMSettingsConnection *self, NMConnection *new); +static void update_agent_secrets_cache(NMSettingsConnection *self, NMConnection *new); +static guint _get_seen_bssids(NMSettingsConnection *self, + const char * strv_buf[static(SEEN_BSSIDS_MAX + 1)]); /*****************************************************************************/ @@ -246,14 +302,6 @@ nm_settings_connection_still_valid(NMSettingsConnection *self) /*****************************************************************************/ -static GHashTable * -_seen_bssids_hash_new(void) -{ - return g_hash_table_new_full(nm_str_hash, g_str_equal, g_free, NULL); -} - -/*****************************************************************************/ - static void _getsettings_cached_clear(NMSettingsConnectionPrivate *priv) { @@ -1300,8 +1348,8 @@ get_settings_auth_cb(NMSettingsConnection * self, GError * error, gpointer data) { - gs_free const char ** seen_bssids = NULL; - NMConnectionSerializationOptions options = {}; + const char * seen_bssids_strv[SEEN_BSSIDS_MAX + 1]; + NMConnectionSerializationOptions options = {}; if (error) { g_dbus_method_invocation_return_gerror(context, error); @@ -1321,8 +1369,8 @@ get_settings_auth_cb(NMSettingsConnection * self, * from the same reason as timestamp. Thus we put it here to GetSettings() * return settings too. */ - seen_bssids = nm_settings_connection_get_seen_bssids(self); - options.seen_bssids = seen_bssids; + _get_seen_bssids(self, seen_bssids_strv); + options.seen_bssids = seen_bssids_strv; /* Secrets should *never* be returned by the GetSettings method, they * get returned by the GetSecrets method which can be better @@ -2304,44 +2352,66 @@ _nm_settings_connection_register_kf_dbs(NMSettingsConnection *self, if (priv->kf_db_seen_bssids != kf_db_seen_bssids) { gs_strfreev char **tmp_strv = NULL; - gsize i, len; + gsize len; + gsize i; + guint result_len; nm_key_file_db_unref(priv->kf_db_seen_bssids); priv->kf_db_seen_bssids = nm_key_file_db_ref(kf_db_seen_bssids); tmp_strv = nm_key_file_db_get_string_list(priv->kf_db_seen_bssids, connection_uuid, &len); - nm_clear_pointer(&priv->seen_bssids, g_hash_table_unref); + if (priv->seen_bssids_hash) + g_hash_table_remove_all(priv->seen_bssids_hash); - if (len > 0) { - _LOGT("read %zu seen-bssids from keyfile database \"%s\"", - len, - nm_key_file_db_get_filename(priv->kf_db_seen_bssids)); - priv->seen_bssids = _seen_bssids_hash_new(); - for (i = len; i > 0;) - g_hash_table_add(priv->seen_bssids, g_steal_pointer(&tmp_strv[--i])); - nm_clear_g_free(&tmp_strv); + for (result_len = 0, i = 0; i < len; i++) { + NMEtherAddr addr_bin; + SeenBssidEntry *entry; + + nm_assert(result_len == nm_g_hash_table_size(priv->seen_bssids_hash)); + if (result_len >= SEEN_BSSIDS_MAX) + break; + + if (!_nm_utils_hwaddr_aton_exact(tmp_strv[i], &addr_bin, sizeof(addr_bin))) + continue; + + if (!priv->seen_bssids_hash) + priv->seen_bssids_hash = _seen_bssids_hash_new(); + + entry = _seen_bssid_entry_new_stale_bin(&addr_bin); + if (!g_hash_table_insert(priv->seen_bssids_hash, entry, entry)) { + /* duplicate detected! The @entry key was freed by g_hash_table_insert(). */ + continue; + } + c_list_link_tail(&priv->seen_bssids_lst_head, &entry->seen_bssids_lst); + result_len++; } + if (result_len > 0) { + _LOGT("read %u seen-bssids from keyfile database \"%s\"", + result_len, + nm_key_file_db_get_filename(priv->kf_db_seen_bssids)); + } else + nm_clear_pointer(&priv->seen_bssids_hash, g_hash_table_destroy); + + nm_assert(nm_g_hash_table_size(priv->seen_bssids_hash) == result_len); + nm_assert(result_len <= SEEN_BSSIDS_MAX); } } -/** - * nm_settings_connection_get_seen_bssids: - * @self: the #NMSettingsConnection - * - * Returns current list of seen BSSIDs for the connection. - * - * Returns: (transfer container) list of seen BSSIDs (in the standard hex-digits-and-colons notation). - * The caller is responsible for freeing the list, but not the content. - **/ -const char ** -nm_settings_connection_get_seen_bssids(NMSettingsConnection *self) +static guint +_get_seen_bssids(NMSettingsConnection *self, const char *strv_buf[static(SEEN_BSSIDS_MAX + 1)]) { - g_return_val_if_fail(NM_IS_SETTINGS_CONNECTION(self), NULL); + NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE(self); + SeenBssidEntry * entry; + guint i; - return nm_utils_strdict_get_keys(NM_SETTINGS_CONNECTION_GET_PRIVATE(self)->seen_bssids, - TRUE, - NULL); + i = 0; + c_list_for_each_entry (entry, &priv->seen_bssids_lst_head, seen_bssids_lst) { + nm_assert(i <= SEEN_BSSIDS_MAX); + strv_buf[i++] = entry->bssid; + } + strv_buf[i] = NULL; + return i; } /** @@ -2355,14 +2425,17 @@ gboolean nm_settings_connection_has_seen_bssid(NMSettingsConnection *self, const char *bssid) { NMSettingsConnectionPrivate *priv; + NMEtherAddr addr_bin; g_return_val_if_fail(NM_IS_SETTINGS_CONNECTION(self), FALSE); g_return_val_if_fail(bssid, FALSE); + nm_assert(_nm_utils_hwaddr_aton_exact(bssid, &addr_bin, sizeof(addr_bin))); priv = NM_SETTINGS_CONNECTION_GET_PRIVATE(self); - return priv->seen_bssids - && g_hash_table_contains(NM_SETTINGS_CONNECTION_GET_PRIVATE(self)->seen_bssids, bssid); + return priv->seen_bssids_hash + && g_hash_table_contains(NM_SETTINGS_CONNECTION_GET_PRIVATE(self)->seen_bssids_hash, + bssid); } /** @@ -2377,15 +2450,47 @@ void nm_settings_connection_add_seen_bssid(NMSettingsConnection *self, const char *seen_bssid) { NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE(self); - gs_free const char ** strv = NULL; + const char * seen_bssids_strv[SEEN_BSSIDS_MAX + 1]; + NMEtherAddr addr_bin; const char * connection_uuid; + SeenBssidEntry entry_stack; + SeenBssidEntry * entry; + guint i; - g_return_if_fail(seen_bssid != NULL); + g_return_if_fail(seen_bssid); - if (!priv->seen_bssids) - priv->seen_bssids = _seen_bssids_hash_new(); + if (!_nm_utils_hwaddr_aton_exact(seen_bssid, &addr_bin, sizeof(addr_bin))) + g_return_if_reached(); - g_hash_table_add(priv->seen_bssids, g_strdup(seen_bssid)); + _seen_bssid_entry_init_stale(&entry_stack, &addr_bin); + + if (!priv->seen_bssids_hash) { + priv->seen_bssids_hash = _seen_bssids_hash_new(); + entry = NULL; + } else + entry = g_hash_table_lookup(priv->seen_bssids_hash, &entry_stack); + + if (entry) { + if (!nm_c_list_move_front(&priv->seen_bssids_lst_head, &entry->seen_bssids_lst)) { + /* no change. */ + return; + } + } else { + entry = _seen_bssid_entry_new_stale_copy(&entry_stack); + c_list_link_front(&priv->seen_bssids_lst_head, &entry->seen_bssids_lst); + if (!g_hash_table_add(priv->seen_bssids_hash, entry)) + nm_assert_not_reached(); + + if (g_hash_table_size(priv->seen_bssids_hash) > SEEN_BSSIDS_MAX) { + g_hash_table_remove( + priv->seen_bssids_hash, + c_list_last_entry(&priv->seen_bssids_lst_head, SeenBssidEntry, seen_bssids_lst)); + } + } + + nm_assert(g_hash_table_size(priv->seen_bssids_hash) <= SEEN_BSSIDS_MAX); + nm_assert(g_hash_table_size(priv->seen_bssids_hash) + == c_list_length(&priv->seen_bssids_lst_head)); if (!priv->kf_db_seen_bssids) return; @@ -2394,12 +2499,8 @@ nm_settings_connection_add_seen_bssid(NMSettingsConnection *self, const char *se if (!connection_uuid) return; - strv = nm_utils_strdict_get_keys(priv->seen_bssids, TRUE, NULL); - - nm_key_file_db_set_string_list(priv->kf_db_seen_bssids, - connection_uuid, - strv ?: NM_PTRARRAY_EMPTY(const char *), - -1); + i = _get_seen_bssids(self, seen_bssids_strv); + nm_key_file_db_set_string_list(priv->kf_db_seen_bssids, connection_uuid, seen_bssids_strv, i); } /*****************************************************************************/ @@ -2610,7 +2711,7 @@ nm_settings_connection_init(NMSettingsConnection *self) self->_priv = priv; c_list_init(&self->_connections_lst); - + c_list_init(&priv->seen_bssids_lst_head); c_list_init(&priv->call_ids_lst_head); c_list_init(&priv->auth_lst_head); @@ -2648,7 +2749,7 @@ dispose(GObject *object) nm_clear_pointer(&priv->agent_secrets, g_variant_unref); - nm_clear_pointer(&priv->seen_bssids, g_hash_table_destroy); + nm_clear_pointer(&priv->seen_bssids_hash, g_hash_table_destroy); g_clear_object(&priv->agent_mgr); diff --git a/src/core/settings/nm-settings-connection.h b/src/core/settings/nm-settings-connection.h index 83a6a7f62b..fa3dbcfbcf 100644 --- a/src/core/settings/nm-settings-connection.h +++ b/src/core/settings/nm-settings-connection.h @@ -340,8 +340,6 @@ gboolean nm_settings_connection_get_timestamp(NMSettingsConnection *self, guint6 void nm_settings_connection_update_timestamp(NMSettingsConnection *self, guint64 timestamp); -const char **nm_settings_connection_get_seen_bssids(NMSettingsConnection *self); - gboolean nm_settings_connection_has_seen_bssid(NMSettingsConnection *self, const char *bssid); void nm_settings_connection_add_seen_bssid(NMSettingsConnection *self, const char *seen_bssid); From 697a445f6b0b73d6366355e6de0b18280c6a604f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Jun 2021 21:42:36 +0200 Subject: [PATCH 08/12] keyfile-aux: add nm_key_file_db_prune() helper A helper function to remove entires that are no longer relevant. (cherry picked from commit f59def45c1b3e9d9892930a424a3f9d576ea9352) --- src/libnm-glib-aux/nm-keyfile-aux.c | 78 ++++++++++++++++++++++++++++- src/libnm-glib-aux/nm-keyfile-aux.h | 4 ++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/libnm-glib-aux/nm-keyfile-aux.c b/src/libnm-glib-aux/nm-keyfile-aux.c index 75abe53848..5d2c0028f7 100644 --- a/src/libnm-glib-aux/nm-keyfile-aux.c +++ b/src/libnm-glib-aux/nm-keyfile-aux.c @@ -27,6 +27,8 @@ struct _NMKeyFileDB { bool dirty : 1; bool destroyed : 1; + bool groups_pruned : 1; + char filename[]; }; @@ -62,6 +64,16 @@ _IS_KEY_FILE_DB(NMKeyFileDB *self, gboolean require_is_started, gboolean allow_d return TRUE; } +static GKeyFile * +_key_file_new(void) +{ + GKeyFile *kf; + + kf = g_key_file_new(); + g_key_file_set_list_separator(kf, ','); + return kf; +} + /*****************************************************************************/ NMKeyFileDB * @@ -86,8 +98,7 @@ nm_key_file_db_new(const char * filename, self->log_fcn = log_fcn; self->got_dirty_fcn = got_dirty_fcn; self->user_data = user_data; - self->kf = g_key_file_new(); - g_key_file_set_list_separator(self->kf, ','); + self->kf = _key_file_new(); memcpy(self->filename, filename, l_filename + 1); self->group_name = &self->filename[l_filename + 1]; memcpy((char *) self->group_name, group_name, l_group + 1); @@ -371,3 +382,66 @@ nm_key_file_db_to_file(NMKeyFileDB *self, gboolean force) } else _LOGD("write keyfile: \"%s\"", self->filename); } + +/*****************************************************************************/ + +void +nm_key_file_db_prune(NMKeyFileDB *self, + gboolean (*predicate)(const char *key, gpointer user_data), + gpointer user_data) +{ + gs_strfreev char ** keys = NULL; + nm_auto_unref_keyfile GKeyFile *kf_to_free = NULL; + GKeyFile * kf_src = NULL; + GKeyFile * kf_dst = NULL; + guint k; + + g_return_if_fail(_IS_KEY_FILE_DB(self, TRUE, FALSE)); + nm_assert(predicate); + + _LOGD("prune keyfile of old entries: \"%s\"", self->filename); + + if (!self->groups_pruned) { + /* When we prune the first time, we swap the GKeyfile instance. + * The instance loaded from disk might have unrelated groups and + * comments. Let's get rid of them by creating a new instance. + * + * Otherwise, we know that self->kf only contains good keys, + * and at most we need to remove some of them. */ + kf_to_free = g_steal_pointer(&self->kf); + self->kf = _key_file_new(); + kf_src = kf_to_free; + self->groups_pruned = TRUE; + self->dirty = TRUE; + } else + kf_src = self->kf; + kf_dst = self->kf; + + keys = g_key_file_get_keys(kf_src, self->group_name, NULL, NULL); + if (keys) { + for (k = 0; keys[k]; k++) { + const char *key = keys[k]; + gboolean keep; + + keep = predicate(key, user_data); + + if (!keep) { + if (kf_dst == kf_src) { + g_key_file_remove_key(kf_dst, self->group_name, key, NULL); + self->dirty = TRUE; + } + continue; + } + + if (kf_dst != kf_src) { + gs_free char *value = NULL; + + value = g_key_file_get_value(kf_src, self->group_name, key, NULL); + if (value) + g_key_file_set_value(kf_dst, self->group_name, key, value); + else + self->dirty = TRUE; + } + } + } +} diff --git a/src/libnm-glib-aux/nm-keyfile-aux.h b/src/libnm-glib-aux/nm-keyfile-aux.h index 72d2f418f9..c4d4e65111 100644 --- a/src/libnm-glib-aux/nm-keyfile-aux.h +++ b/src/libnm-glib-aux/nm-keyfile-aux.h @@ -50,6 +50,10 @@ void nm_key_file_db_set_string_list(NMKeyFileDB * self, void nm_key_file_db_to_file(NMKeyFileDB *self, gboolean force); +void nm_key_file_db_prune(NMKeyFileDB *self, + gboolean (*predicate)(const char *key, gpointer user_data), + gpointer user_data); + /*****************************************************************************/ #endif /* __NM_KEYFILE_AUX_H__ */ From 006733d9b101c63b3cfb6f4c859a127e6ad4e76d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Jun 2021 21:47:15 +0200 Subject: [PATCH 09/12] settings: prune old entries from keyfile databases We have two GKeyfile files (timestamps and seen-bssids). When a profile was deleted while NetworkManager was running, then entries were removed from these keyfiles. But if a profile disappeared while NetworkManger was stopped, then those UUIDs piled up. This also happens if you have temporary connections in /run and reboot. We need a way to garbage collect entries that are no longer relevant. As the keyfile databases only get loaded once from disk, we will prune all UUIDs for which we have no more connection loaded, on the first time we write out the files again. Note what this means: if you "temporarily" remove a connection profile (without NetworkManager noticing) and restore it later, then the additional information might have been pruned. There is no way how NetworkManager could know that this UUID is coming back. The alternative is what we did before: pile them up indefinitely. That seems more problematic. (cherry picked from commit 2e720a1dc8b488969d80150487a733d462ccfacb) --- src/core/settings/nm-settings.c | 49 ++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/src/core/settings/nm-settings.c b/src/core/settings/nm-settings.c index c876ea148c..631b795df1 100644 --- a/src/core/settings/nm-settings.c +++ b/src/core/settings/nm-settings.c @@ -388,6 +388,9 @@ typedef struct { guint kf_db_flush_idle_id_timestamps; guint kf_db_flush_idle_id_seen_bssids; + bool kf_db_pruned_timestamps; + bool kf_db_pruned_seen_bssid; + bool started : 1; /* Whether NMSettingsConnections changed in a way that affects the comparison @@ -3684,6 +3687,37 @@ again: /*****************************************************************************/ +static gboolean +_kf_db_prune_predicate(const char *uuid, gpointer user_data) +{ + return !!nm_settings_get_connection_by_uuid(user_data, uuid); +} + +static void +_kf_db_to_file(NMSettings *self, gboolean is_timestamps, gboolean force_write) +{ + NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE(self); + NMKeyFileDB * kf_db; + bool * p_kf_db_pruned; + + if (is_timestamps) { + kf_db = priv->kf_db_timestamps; + p_kf_db_pruned = &priv->kf_db_pruned_timestamps; + } else { + kf_db = priv->kf_db_seen_bssids; + p_kf_db_pruned = &priv->kf_db_pruned_seen_bssid; + } + + if (!*p_kf_db_pruned) { + /* we only prune the DB once, because afterwards every + * add/remove of an connection will lead to a direct update. */ + *p_kf_db_pruned = TRUE; + nm_key_file_db_prune(kf_db, _kf_db_prune_predicate, self); + } + + nm_key_file_db_to_file(kf_db, force_write); +} + G_GNUC_PRINTF(4, 5) static void _kf_db_log_fcn(NMKeyFileDB *kf_db, int syslog_level, gpointer user_data, const char *fmt, ...) @@ -3732,7 +3766,7 @@ _kf_db_got_dirty_flush(NMSettings *self, gboolean is_timestamps) } if (nm_key_file_db_is_dirty(kf_db)) - nm_key_file_db_to_file(kf_db, FALSE); + _kf_db_to_file(self, is_timestamps, FALSE); else { _LOGT("[%s-keyfile]: skip saving changes to \"%s\"", prefix, @@ -3785,15 +3819,10 @@ _kf_db_got_dirty_fcn(NMKeyFileDB *kf_db, gpointer user_data) void nm_settings_kf_db_write(NMSettings *self) { - NMSettingsPrivate *priv; - g_return_if_fail(NM_IS_SETTINGS(self)); - priv = NM_SETTINGS_GET_PRIVATE(self); - if (priv->kf_db_timestamps) - nm_key_file_db_to_file(priv->kf_db_timestamps, TRUE); - if (priv->kf_db_seen_bssids) - nm_key_file_db_to_file(priv->kf_db_seen_bssids, TRUE); + _kf_db_to_file(self, TRUE, TRUE); + _kf_db_to_file(self, FALSE, TRUE); } /*****************************************************************************/ @@ -4031,8 +4060,8 @@ finalize(GObject *object) nm_clear_g_source(&priv->kf_db_flush_idle_id_timestamps); nm_clear_g_source(&priv->kf_db_flush_idle_id_seen_bssids); - nm_key_file_db_to_file(priv->kf_db_timestamps, FALSE); - nm_key_file_db_to_file(priv->kf_db_seen_bssids, FALSE); + _kf_db_to_file(self, TRUE, FALSE); + _kf_db_to_file(self, FALSE, FALSE); nm_key_file_db_destroy(priv->kf_db_timestamps); nm_key_file_db_destroy(priv->kf_db_seen_bssids); From 567499cddd3f31c708a8a4fdd3135ba2c8c44ab2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Jun 2021 09:48:48 +0200 Subject: [PATCH 10/12] glib-aux: add nm_utils_find_mkstemp_files() (cherry picked from commit 080d7654476c0e7cc4c0a5baa801a2d22972ed1d) --- src/libnm-glib-aux/nm-io-utils.c | 64 ++++++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-io-utils.h | 2 + 2 files changed, 66 insertions(+) diff --git a/src/libnm-glib-aux/nm-io-utils.c b/src/libnm-glib-aux/nm-io-utils.c index 894c8726c0..87478a2dc2 100644 --- a/src/libnm-glib-aux/nm-io-utils.c +++ b/src/libnm-glib-aux/nm-io-utils.c @@ -564,3 +564,67 @@ nm_g_subprocess_terminate_in_background(GSubprocess *subprocess, int timeout_mse NULL), main_context); } + +/*****************************************************************************/ + +char ** +nm_utils_find_mkstemp_files(const char *dirname, const char *filename) +{ + static const char letters[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + DIR * dir; + struct dirent * entry; + GPtrArray * arr = NULL; + gsize l; + + /* We write files with g_file_set_contents() and nm_utils_file_set_contents(). + * These create temporary files using g_mkstemp_full(), with a random .XXXXXX suffix. + * + * If NetworkManager crashes while writing the file, then those temporary files are + * left over. We might want to find and delete such files. + * + * Beware: only delete such files if you are in full control about which files are + * supposed to be in the directory. For example, NetworkManager controls + * /var/lib/NetworkManager/timestamps files, and it thus takes the right to delete + * all files /var/lib/NetworkManager/timestamps.XXXXXX. That may not be appropriate + * in other cases! */ + + if (!dirname || !filename || !filename[0]) + return NULL; + + dir = opendir(dirname); + if (!dir) + return NULL; + + l = strlen(filename); + + while ((entry = readdir(dir))) { + const char *f = entry->d_name; + guint i; + + if (strncmp(f, filename, l) != 0) + goto next; + if (f[l] != '.') + goto next; + for (i = 1; i <= 6; i++) { + /* @letters is also what g_mkstemp_full() does! */ + if (!memchr(letters, f[l + i], G_N_ELEMENTS(letters))) + goto next; + } + if (f[l + 7] != '\0') + goto next; + + if (!arr) + arr = g_ptr_array_new(); + + g_ptr_array_add(arr, g_strdup(f)); +next:; + } + + closedir(dir); + + if (!arr) + return NULL; + + g_ptr_array_add(arr, NULL); + return (char **) g_ptr_array_free(arr, FALSE); +} diff --git a/src/libnm-glib-aux/nm-io-utils.h b/src/libnm-glib-aux/nm-io-utils.h index 98e63ac01e..31ff6d0569 100644 --- a/src/libnm-glib-aux/nm-io-utils.h +++ b/src/libnm-glib-aux/nm-io-utils.h @@ -58,4 +58,6 @@ int nm_utils_file_stat(const char *filename, struct stat *out_st); void nm_g_subprocess_terminate_in_background(GSubprocess *subprocess, int timeout_msec_before_kill); +char **nm_utils_find_mkstemp_files(const char *dirname, const char *filename); + #endif /* __NM_IO_UTILS_H__ */ From 4b942e95194b858902137839fddf9654d7eb48e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Jun 2021 09:54:17 +0200 Subject: [PATCH 11/12] glib-aux: add nm_key_file_db_prune_tmp_files() helper (cherry picked from commit 3c0f1eb0fd15109fbac29b23d4240a93c0809333) --- src/libnm-glib-aux/nm-keyfile-aux.c | 40 +++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-keyfile-aux.h | 2 ++ 2 files changed, 42 insertions(+) diff --git a/src/libnm-glib-aux/nm-keyfile-aux.c b/src/libnm-glib-aux/nm-keyfile-aux.c index 5d2c0028f7..9cda1cf714 100644 --- a/src/libnm-glib-aux/nm-keyfile-aux.c +++ b/src/libnm-glib-aux/nm-keyfile-aux.c @@ -385,6 +385,46 @@ nm_key_file_db_to_file(NMKeyFileDB *self, gboolean force) /*****************************************************************************/ +void +nm_key_file_db_prune_tmp_files(NMKeyFileDB *self) +{ + gs_free char * n_file = NULL; + gs_free char * n_dir = NULL; + gs_strfreev char **tmpfiles = NULL; + gsize i; + + n_file = g_path_get_basename(self->filename); + n_dir = g_path_get_dirname(self->filename); + + tmpfiles = nm_utils_find_mkstemp_files(n_dir, n_file); + if (!tmpfiles) + return; + + for (i = 0; tmpfiles[i]; i++) { + const char * tmpfile = tmpfiles[i]; + gs_free char *full_file = NULL; + int r; + + full_file = g_strdup_printf("%s/%s", n_dir, tmpfile); + + r = unlink(full_file); + if (r != 0) { + int errsv = errno; + + if (errsv != ENOENT) { + _LOGD("prune left over temp file %s failed: %s", + full_file, + nm_strerror_native(errsv)); + } + continue; + } + + _LOGD("prune left over temp file %s", full_file); + } +} + +/*****************************************************************************/ + void nm_key_file_db_prune(NMKeyFileDB *self, gboolean (*predicate)(const char *key, gpointer user_data), diff --git a/src/libnm-glib-aux/nm-keyfile-aux.h b/src/libnm-glib-aux/nm-keyfile-aux.h index c4d4e65111..e756c57a84 100644 --- a/src/libnm-glib-aux/nm-keyfile-aux.h +++ b/src/libnm-glib-aux/nm-keyfile-aux.h @@ -50,6 +50,8 @@ void nm_key_file_db_set_string_list(NMKeyFileDB * self, void nm_key_file_db_to_file(NMKeyFileDB *self, gboolean force); +void nm_key_file_db_prune_tmp_files(NMKeyFileDB *self); + void nm_key_file_db_prune(NMKeyFileDB *self, gboolean (*predicate)(const char *key, gpointer user_data), gpointer user_data); From 35402a7e909c9507698261478f97faf8592eebf3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Jun 2021 10:14:13 +0200 Subject: [PATCH 12/12] settings: cleanup left over temporary files for timestamps/seen-bssids (cherry picked from commit 34c663ca1af1aca3d11d53a75a029eb4a8974770) --- src/core/settings/nm-settings.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/settings/nm-settings.c b/src/core/settings/nm-settings.c index 631b795df1..a81f76b539 100644 --- a/src/core/settings/nm-settings.c +++ b/src/core/settings/nm-settings.c @@ -3713,6 +3713,10 @@ _kf_db_to_file(NMSettings *self, gboolean is_timestamps, gboolean force_write) * add/remove of an connection will lead to a direct update. */ *p_kf_db_pruned = TRUE; nm_key_file_db_prune(kf_db, _kf_db_prune_predicate, self); + + /* once we also go over the directory, and see whether we + * have any left over temporary files to delete. */ + nm_key_file_db_prune_tmp_files(kf_db); } nm_key_file_db_to_file(kf_db, force_write);