From cd2cef9cab62db042f646bf0fcdc318106fc627f Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Thu, 19 Feb 2015 17:53:43 +0100 Subject: [PATCH 1/5] utils: match a cloned mac address with a connection that does not specify it We do the same for the original MAC address. A device enslaved to a bond it inherits the bond's MAC address. When NetworkManager tries to assume a connection the generated cloned-mac property causes a mismatch with the connection that originally brought up the device, causing the generated connection to be used instead: NetworkManager[14190]: [1424355817.112154] [NetworkManagerUtils.c:1641] nm_utils_match_connection(): Connection 'eth2' differs from candidate 'bond-slave-eth2' in 802-3-ethernet.cloned-mac-address https://bugzilla.gnome.org/show_bug.cgi?id=744812 --- src/NetworkManagerUtils.c | 36 +++++++++++++++++++++++++++++++ src/tests/test-general.c | 45 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index a5de386652..0f957814d3 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -1646,6 +1646,39 @@ check_connection_mac_address (NMConnection *orig, return FALSE; } +static gboolean +check_connection_cloned_mac_address (NMConnection *orig, + NMConnection *candidate, + GHashTable *settings) +{ + GHashTable *props; + const char *orig_mac = NULL, *cand_mac = NULL; + NMSettingWired *s_wired_orig, *s_wired_cand; + + props = check_property_in_hash (settings, + NM_SETTING_WIRED_SETTING_NAME, + NM_SETTING_WIRED_CLONED_MAC_ADDRESS); + if (!props) + return TRUE; + + /* If one of the MAC addresses is NULL, we accept that connection */ + s_wired_orig = nm_connection_get_setting_wired (orig); + if (s_wired_orig) + orig_mac = nm_setting_wired_get_cloned_mac_address (s_wired_orig); + + s_wired_cand = nm_connection_get_setting_wired (candidate); + if (s_wired_cand) + cand_mac = nm_setting_wired_get_cloned_mac_address (s_wired_cand); + + if (!orig_mac || !cand_mac) { + remove_from_hash (settings, props, + NM_SETTING_WIRED_SETTING_NAME, + NM_SETTING_WIRED_CLONED_MAC_ADDRESS); + return TRUE; + } + return FALSE; +} + static NMConnection * check_possible_match (NMConnection *orig, NMConnection *candidate, @@ -1666,6 +1699,9 @@ check_possible_match (NMConnection *orig, if (!check_connection_mac_address (orig, candidate, settings)) return NULL; + if (!check_connection_cloned_mac_address (orig, candidate, settings)) + return NULL; + if (g_hash_table_size (settings) == 0) return candidate; else diff --git a/src/tests/test-general.c b/src/tests/test-general.c index dae872c86d..baea1d54e2 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -439,6 +439,50 @@ test_connection_match_wired (void) g_object_unref (copy); } +static void +test_connection_match_cloned_mac (void) +{ + NMConnection *orig, *exact, *fuzzy, *matched; + GSList *connections = NULL; + NMSettingWired *s_wired; + + orig = _match_connection_new (); + + fuzzy = nm_simple_connection_new_clone (orig); + connections = g_slist_append (connections, fuzzy); + s_wired = nm_connection_get_setting_wired (orig); + g_assert (s_wired); + g_object_set (G_OBJECT (s_wired), + NM_SETTING_WIRED_CLONED_MAC_ADDRESS, "52:54:00:ab:db:23", + NULL); + + matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + g_assert (matched == fuzzy); + + exact = nm_simple_connection_new_clone (orig); + connections = g_slist_append (connections, exact); + s_wired = nm_connection_get_setting_wired (exact); + g_assert (s_wired); + g_object_set (G_OBJECT (s_wired), + NM_SETTING_WIRED_CLONED_MAC_ADDRESS, "52:54:00:ab:db:23", + NULL); + + matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + g_assert (matched == exact); + + g_object_set (G_OBJECT (s_wired), + NM_SETTING_WIRED_CLONED_MAC_ADDRESS, "52:54:00:ab:db:24", + NULL); + + matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + g_assert (matched == fuzzy); + + g_slist_free (connections); + g_object_unref (orig); + g_object_unref (fuzzy); + g_object_unref (exact); +} + static void test_connection_no_match_ip4_addr (void) { @@ -727,6 +771,7 @@ main (int argc, char **argv) g_test_add_func ("/general/connection-match/ip4-method", test_connection_match_ip4_method); g_test_add_func ("/general/connection-match/con-interface-name", test_connection_match_interface_name); g_test_add_func ("/general/connection-match/wired", test_connection_match_wired); + g_test_add_func ("/general/connection-match/cloned_mac", test_connection_match_cloned_mac); g_test_add_func ("/general/connection-match/no-match-ip4-addr", test_connection_no_match_ip4_addr); g_test_add_func ("/general/connection-sort/autoconnect-priority", test_connection_sort_autoconnect_priority); From 8a00bb36ec9091a6438b02faf324fac3befe0016 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 22 Apr 2015 15:05:53 +0200 Subject: [PATCH 2/5] nm-settings: add nm_settings_has_connection() Useful for checking if a connection is already deleted. https://bugzilla.gnome.org/show_bug.cgi?id=744812 https://bugzilla.redhat.com/show_bug.cgi?id=1174164 --- src/settings/nm-settings.c | 15 +++++++++++++++ src/settings/nm-settings.h | 2 ++ 2 files changed, 17 insertions(+) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 4a03f4a5f2..60f98948d5 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -467,6 +467,21 @@ notify (GObject *object, GParamSpec *pspec) g_slice_free (GValue, value); } +gboolean +nm_settings_has_connection (NMSettings *self, NMConnection *connection) +{ + NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); + GHashTableIter iter; + gpointer data; + + g_hash_table_iter_init (&iter, priv->connections); + while (g_hash_table_iter_next (&iter, NULL, &data)) + if (data == connection) + return TRUE; + + return FALSE; +} + const GSList * nm_settings_get_unmanaged_specs (NMSettings *self) { diff --git a/src/settings/nm-settings.h b/src/settings/nm-settings.h index 9c0a4c835e..d196efe655 100644 --- a/src/settings/nm-settings.h +++ b/src/settings/nm-settings.h @@ -111,6 +111,8 @@ NMSettingsConnection *nm_settings_get_connection_by_path (NMSettings *settings, NMSettingsConnection *nm_settings_get_connection_by_uuid (NMSettings *settings, const char *uuid); +gboolean nm_settings_has_connection (NMSettings *self, NMConnection *connection); + const GSList *nm_settings_get_unmanaged_specs (NMSettings *self); char *nm_settings_get_hostname (NMSettings *self); From 74ed416d845ecea6684a39701d6aea5f7ba9029e Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 22 Apr 2015 15:05:18 +0200 Subject: [PATCH 3/5] manager: don't try to delete generated connection if it's already gone Move the cleanup of the generated assumed connection to active connection dispose. If the connection vanishes earlier (explicit deletion from client), tear down the reference so that we don't try to remove it redundantly. NetworkManager[9221]: (eth2): device state change: deactivating -> disconnected (reason 'connection-removed') [110 30 38] NetworkManager[9221]: (eth2): deactivating device (reason 'connection-removed') [38] (NetworkManager:9221): GLib-GObject-WARNING **: g_object_weak_unref: couldn't find weak ref 0x496610(0x7c2ba0) Program received signal SIGTRAP, Trace/breakpoint trap. g_logv (log_domain=0x7ffff4d4f1a4 "GLib-GObject", log_level=G_LOG_LEVEL_WARNING, format=, args=args@entry=0x7fffffffd860) at gmessages.c:1046 1046 g_private_set (&g_log_depth, GUINT_TO_POINTER (depth)); (gdb) bt #0 0x00007ffff4a2cc60 in g_logv (log_domain=0x7ffff4d4f1a4 "GLib-GObject", log_level=G_LOG_LEVEL_WARNING, format=, args=args@entry=0x7fffffffd860) at gmessages.c:1046 #1 0x00007ffff4a2ce9f in g_log (log_domain=, log_level=, format=) at gmessages.c:1079 #2 0x000000000049780b in nm_dbus_manager_unregister_object (self=0x7c2ba0 [NMDBusManager], object=0x80f3e0) at nm-dbus-manager.c:921 #3 0x000000000047cc83 in nm_settings_connection_signal_remove (self=self@entry=0x80f3e0 [NMIfcfgConnection]) at settings/nm-settings-connection.c:1752 #4 0x000000000047cd22 in do_delete (connection=0x80f3e0 [NMIfcfgConnection], callback=0x479d60 , user_data=0x0) at settings/nm-settings-connection.c:687 #5 0x00000000004b1eb6 in active_connection_remove (self=self@entry=0x8701c0 [NMManager], active=active@entry=0x8b02f0) at nm-manager.c:292 #6 0x00000000004b2174 in _active_connection_cleanup (user_data=) at nm-manager.c:316 #7 0x00007ffff4a25aeb in g_main_context_dispatch (context=0x7be3a0) at gmain.c:3111 #8 0x00007ffff4a25aeb in g_main_context_dispatch (context=context@entry=0x7be3a0) at gmain.c:3710 #9 0x00007ffff4a25e88 in g_main_context_iterate (context=0x7be3a0, block=block@entry=1, dispatch=dispatch@entry=1, self=) at gmain.c:3781 #10 0x00007ffff4a261b2 in g_main_loop_run (loop=0x7be460) at gmain.c:3975 #11 0x0000000000432f55 in main (argc=1, argv=0x7fffffffded8) at main.c:460 (gdb) https://bugzilla.gnome.org/show_bug.cgi?id=744812 --- src/nm-manager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 7888e68f7c..a9dd345465 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -286,7 +286,8 @@ active_connection_remove (NMManager *self, NMActiveConnection *active) g_object_unref (active); - if (connection) { + if ( connection + && nm_settings_has_connection (priv->settings, connection)) { nm_log_dbg (LOGD_DEVICE, "Assumed connection disconnected. Deleting generated connection '%s' (%s)", nm_connection_get_id (connection), nm_connection_get_uuid (connection)); nm_settings_connection_delete (NM_SETTINGS_CONNECTION (connection), NULL, NULL); From fe96dbc0ee15b7df7bf6920bed2a054304059ae2 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Thu, 23 Apr 2015 09:40:42 -0500 Subject: [PATCH 4/5] settings/dbus: harden connection removal and object unexport None of these functions was checking if the same operation had already been performed, or if the object being removed/unexported was known. --- src/settings/nm-settings-connection.c | 8 ++++++++ src/settings/nm-settings.c | 8 ++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 9285dfbfa5..fae0f025db 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -96,6 +96,8 @@ enum { static guint signals[LAST_SIGNAL] = { 0 }; typedef struct { + gboolean removed; + NMAgentManager *agent_mgr; guint session_changed_id; @@ -1743,6 +1745,12 @@ impl_settings_connection_clear_secrets (NMSettingsConnection *self, void nm_settings_connection_signal_remove (NMSettingsConnection *self) { + NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self); + + if (priv->removed) + g_return_if_reached (); + priv->removed = TRUE; + /* Emit removed first */ g_signal_emit_by_name (self, NM_SETTINGS_CONNECTION_REMOVED); diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 60f98948d5..9ba0e218c0 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -808,6 +808,11 @@ static void connection_removed (NMSettingsConnection *connection, gpointer user_data) { NMSettings *self = NM_SETTINGS (user_data); + NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); + const char *cpath = nm_connection_get_path (NM_CONNECTION (connection)); + + if (!g_hash_table_lookup (priv->connections, cpath)) + g_return_if_reached (); g_object_ref (connection); @@ -823,8 +828,7 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) g_signal_handlers_disconnect_by_func (connection, G_CALLBACK (connection_ready_changed), self); /* Forget about the connection internally */ - g_hash_table_remove (NM_SETTINGS_GET_PRIVATE (user_data)->connections, - (gpointer) nm_connection_get_path (NM_CONNECTION (connection))); + g_hash_table_remove (priv->connections, (gpointer) cpath); /* Notify D-Bus */ g_signal_emit (self, signals[CONNECTION_REMOVED], 0, connection); From bbcf5444fd83bbff6124ad90ab4f1ce29ce13e1c Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Thu, 23 Apr 2015 09:43:20 -0500 Subject: [PATCH 5/5] settings: remove 'do_export' argument from claim_connection() It was always TRUE, and unused anyway. --- src/settings/nm-settings.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 9ba0e218c0..6100681502 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -95,8 +95,7 @@ EXPORT(nm_settings_connection_replace_and_commit) /* END LINKER CRACKROCK */ static void claim_connection (NMSettings *self, - NMSettingsConnection *connection, - gboolean do_export); + NMSettingsConnection *connection); static gboolean impl_settings_list_connections (NMSettings *self, GPtrArray **connections, @@ -218,7 +217,7 @@ plugin_connection_added (NMSystemConfigInterface *config, NMSettingsConnection *connection, gpointer user_data) { - claim_connection (NM_SETTINGS (user_data), connection, TRUE); + claim_connection (NM_SETTINGS (user_data), connection); } static void @@ -239,7 +238,7 @@ load_connections (NMSettings *self) // priority plugin. for (elt = plugin_connections; elt; elt = g_slist_next (elt)) - claim_connection (self, NM_SETTINGS_CONNECTION (elt->data), TRUE); + claim_connection (self, NM_SETTINGS_CONNECTION (elt->data)); g_slist_free (plugin_connections); @@ -895,9 +894,7 @@ openconnect_migrate_hack (NMConnection *connection) } static void -claim_connection (NMSettings *self, - NMSettingsConnection *connection, - gboolean do_export) +claim_connection (NMSettings *self, NMSettingsConnection *connection) { NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); static guint32 ec_counter = 0; @@ -1048,7 +1045,7 @@ nm_settings_add_connection (NMSettings *self, added = nm_system_config_interface_add_connection (plugin, connection, save_to_disk, &add_error); if (added) { - claim_connection (self, added, TRUE); + claim_connection (self, added); return added; } nm_log_dbg (LOGD_SETTINGS, "Failed to add %s/'%s': %s",