From d6837f0bd30da069d327099cb555854630cd4584 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 2 May 2024 16:40:26 +0200 Subject: [PATCH 1/2] settings: add nm_settings_connection_persist_mode_to_string() (cherry picked from commit a48b7fe7b9d8adf4902c7b3cfcc4d89bc46cbbef) (cherry picked from commit e5837aa1d3960b743adcff0a5041445ccd65fb93) --- src/core/settings/nm-settings-connection.c | 23 ++++++++++++++++++++++ src/core/settings/nm-settings-connection.h | 4 ++++ 2 files changed, 27 insertions(+) diff --git a/src/core/settings/nm-settings-connection.c b/src/core/settings/nm-settings-connection.c index 176cc2c252..459c60ad1e 100644 --- a/src/core/settings/nm-settings-connection.c +++ b/src/core/settings/nm-settings-connection.c @@ -226,6 +226,29 @@ static guint _get_seen_bssids(NMSettingsConnection *self, /*****************************************************************************/ +char * +nm_settings_connection_persist_mode_to_string(NMSettingsConnectionPersistMode mode) +{ + switch (mode) { + case NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY: + return "in-memory"; + case NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED: + return "in-memory-detached"; + case NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY: + return "in-memory-only"; + case NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP: + return "keep"; + case NM_SETTINGS_CONNECTION_PERSIST_MODE_NO_PERSIST: + return "no-persist"; + case NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK: + return "to-disk"; + } + + return nm_assert_unreachable_val(NULL); +} + +/*****************************************************************************/ + NMSettings * nm_settings_connection_get_settings(NMSettingsConnection *self) { diff --git a/src/core/settings/nm-settings-connection.h b/src/core/settings/nm-settings-connection.h index 835a978e40..d15a75b749 100644 --- a/src/core/settings/nm-settings-connection.h +++ b/src/core/settings/nm-settings-connection.h @@ -379,4 +379,8 @@ void _nm_settings_connection_emit_signal_updated_internal( void _nm_settings_connection_cleanup_after_remove(NMSettingsConnection *self); +/*****************************************************************************/ + +char *nm_settings_connection_persist_mode_to_string(NMSettingsConnectionPersistMode mode); + #endif /* __NETWORKMANAGER_SETTINGS_CONNECTION_H__ */ -- 2.41.0 From c6f9d0a6d5c864ba0141b6e985727cd69c5560fa Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 15 Apr 2024 10:51:24 +0200 Subject: [PATCH 2/2] checkpoint: preserve in-memory state of connections If a connection is in-memory (i.e. has flag "unsaved"), after a checkpoint and rollback it can be wrongly persisted to disk: - if the connection was modified and written to disk after the rollback, during the rollback we update it again with persist mode "keep", which keeps it on disk; - if the connection was deleted after the rollback, during the rollback we add it again with persist mode "to-disk". Instead, remember whether the connection had the "unsaved" flag set and try to restore the previous state. However, this is not straightforward as there are 4 different possible states for the settings connection: persistent; in-memory only; in-memory shadowing a persistent file; in-memory shadowing a detached persistent file (i.e. the deletion of the connection doesn't delete the persistent file). Handle all those cases. Fixes: 3e09aed2a09f ('checkpoint: add create, rollback and destroy D-Bus API') (cherry picked from commit c979bfeb8b0d3bed19bac2ad01a6a6ed899f924e) (cherry picked from commit ebf25794d9cd89190775ac401c36d63aa1c108f7) --- NEWS | 8 ++ src/core/nm-checkpoint.c | 242 ++++++++++++++++++++++++++++++++------- 2 files changed, 211 insertions(+), 39 deletions(-) diff --git a/NEWS b/NEWS index 6ac3118db9..e33152c6f4 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,11 @@ +=============================================== +NetworkManager-1.46.2 +Overview of changes since NetworkManager-1.46.0 +=============================================== + +* Properly restore in-memory connection profiles during the rollback + of a checkpoint. + ============================================= NetworkManager-1.46 Overview of changes since NetworkManager-1.44 diff --git a/src/core/nm-checkpoint.c b/src/core/nm-checkpoint.c index cc5c189bf9..ffcf6e3aad 100644 --- a/src/core/nm-checkpoint.c +++ b/src/core/nm-checkpoint.c @@ -10,6 +10,7 @@ #include "nm-active-connection.h" #include "nm-act-request.h" #include "libnm-core-aux-intern/nm-auth-subject.h" +#include "libnm-core-intern/nm-keyfile-internal.h" #include "nm-core-utils.h" #include "nm-dbus-interface.h" #include "devices/nm-device.h" @@ -17,6 +18,7 @@ #include "nm-manager.h" #include "settings/nm-settings.h" #include "settings/nm-settings-connection.h" +#include "settings/plugins/keyfile/nms-keyfile-storage.h" #include "nm-simple-connection.h" #include "nm-utils.h" @@ -29,11 +31,14 @@ typedef struct { NMDevice *device; NMConnection *applied_connection; NMConnection *settings_connection; + NMConnection *settings_connection_shadowed; guint64 ac_version_id; NMDeviceState state; bool is_software : 1; bool realized : 1; bool activation_lifetime_bound_to_profile_visibility : 1; + bool settings_connection_is_unsaved : 1; + bool settings_connection_is_shadowed_owned : 1; NMUnmanFlagOp unmanaged_explicit; NMActivationReason activation_reason; gulong dev_exported_change_id; @@ -150,37 +155,111 @@ nm_checkpoint_includes_devices_of(NMCheckpoint *self, NMCheckpoint *cp_for_devic return NULL; } +static NMConnection * +parse_connection_from_shadowed_file(const char *path, GError **error) +{ + nm_auto_unref_keyfile GKeyFile *keyfile = NULL; + gs_free char *base_dir = NULL; + char *sep; + + keyfile = g_key_file_new(); + if (!g_key_file_load_from_file(keyfile, path, G_KEY_FILE_NONE, error)) + return NULL; + + sep = strrchr(path, '/'); + base_dir = g_strndup(path, sep - path); + + return nm_keyfile_read(keyfile, base_dir, NM_KEYFILE_HANDLER_FLAGS_NONE, NULL, NULL, error); +} + static NMSettingsConnection * -find_settings_connection(NMCheckpoint *self, - DeviceCheckpoint *dev_checkpoint, - gboolean *need_update, - gboolean *need_activation) +find_settings_connection(NMCheckpoint *self, + DeviceCheckpoint *dev_checkpoint, + gboolean *need_update, + gboolean *need_update_shadowed, + gboolean *need_activation, + NMSettingsConnectionPersistMode *persist_mode) { NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE(self); NMActiveConnection *active; NMSettingsConnection *sett_conn; + const char *shadowed_file; + NMConnection *shadowed_connection = NULL; const char *uuid, *ac_uuid; const CList *tmp_clist; - - *need_activation = FALSE; - *need_update = FALSE; + gboolean sett_conn_unsaved; + NMSettingsStorage *storage; + + *need_activation = FALSE; + *need_update = FALSE; + *need_update_shadowed = FALSE; + + /* With regard to storage, there are 4 different possible states for the settings + * connection: 1) persistent; 2) in-memory only; 3) in-memory shadowing a persistent + * file; 4) in-memory shadowing a detached persistent file (i.e. the deletion of + * the connection doesn't delete the persistent file). + */ + if (dev_checkpoint->settings_connection_is_unsaved) { + if (dev_checkpoint->settings_connection_shadowed) { + if (dev_checkpoint->settings_connection_is_shadowed_owned) + *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY; + else + *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED; + } else + *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY; + } else { + *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; + } uuid = nm_connection_get_uuid(dev_checkpoint->settings_connection); sett_conn = nm_settings_get_connection_by_uuid(NM_SETTINGS_GET, uuid); - if (!sett_conn) - return NULL; - - /* Now check if the connection changed, ... */ - if (!nm_connection_compare(dev_checkpoint->settings_connection, - nm_settings_connection_get_connection(sett_conn), - NM_SETTING_COMPARE_FLAG_EXACT)) { + /* Check if the connection changed */ + if (sett_conn + && !nm_connection_compare(dev_checkpoint->settings_connection, + nm_settings_connection_get_connection(sett_conn), + NM_SETTING_COMPARE_FLAG_IGNORE_TIMESTAMP)) { _LOGT("rollback: settings connection %s changed", uuid); *need_update = TRUE; *need_activation = TRUE; } - /* ... is active, ... */ + storage = sett_conn ? nm_settings_connection_get_storage(sett_conn) : NULL; + shadowed_file = storage ? nm_settings_storage_get_shadowed_storage(storage, NULL) : NULL; + shadowed_connection = + shadowed_file ? parse_connection_from_shadowed_file(shadowed_file, NULL) : NULL; + + if (dev_checkpoint->settings_connection_shadowed) { + if (!shadowed_connection + || !nm_connection_compare(dev_checkpoint->settings_connection_shadowed, + shadowed_connection, + NM_SETTING_COMPARE_FLAG_IGNORE_TIMESTAMP)) { + _LOGT("rollback: shadowed connection changed for %s", uuid); + *need_update_shadowed = TRUE; + *need_update = TRUE; + } + } else { + if (shadowed_connection) { + _LOGT("rollback: shadowed connection changed for %s", uuid); + *need_update = TRUE; + } + } + + if (!sett_conn) + return NULL; + + /* Check if the connection unsaved flag changed */ + sett_conn_unsaved = NM_FLAGS_HAS(nm_settings_connection_get_flags(sett_conn), + NM_SETTINGS_CONNECTION_INT_FLAGS_UNSAVED); + if (sett_conn_unsaved != dev_checkpoint->settings_connection_is_unsaved) { + _LOGT("rollback: storage changed for settings connection %s: unsaved (%d -> %d)", + uuid, + dev_checkpoint->settings_connection_is_unsaved, + sett_conn_unsaved); + *need_update = TRUE; + } + + /* Check if the active state changed */ nm_manager_for_each_active_connection (priv->manager, active, tmp_clist) { ac_uuid = nm_settings_connection_get_uuid(nm_active_connection_get_settings_connection(active)); @@ -196,7 +275,7 @@ find_settings_connection(NMCheckpoint *self, return sett_conn; } - /* ... or if the connection was reactivated/reapplied */ + /* Check if the connection was reactivated/reapplied */ if (nm_active_connection_version_id_get(active) != dev_checkpoint->ac_version_id) { _LOGT("rollback: active connection version id of %s changed", uuid); *need_activation = TRUE; @@ -212,12 +291,19 @@ restore_and_activate_connection(NMCheckpoint *self, DeviceCheckpoint *dev_checkp NMSettingsConnection *connection; gs_unref_object NMAuthSubject *subject = NULL; GError *local_error = NULL; - gboolean need_update, need_activation; + gboolean need_update; + gboolean need_update_shadowed; + gboolean need_activation; NMSettingsConnectionPersistMode persist_mode; NMSettingsConnectionIntFlags sett_flags; NMSettingsConnectionIntFlags sett_mask; - connection = find_settings_connection(self, dev_checkpoint, &need_update, &need_activation); + connection = find_settings_connection(self, + dev_checkpoint, + &need_update, + &need_update_shadowed, + &need_activation, + &persist_mode); /* FIXME: we need to ensure to re-create/update the profile for the * same settings plugin. E.g. if it was a keyfile in /run or /etc, @@ -229,9 +315,26 @@ restore_and_activate_connection(NMCheckpoint *self, DeviceCheckpoint *dev_checkp sett_mask = NM_SETTINGS_CONNECTION_INT_FLAGS_NONE; if (connection) { + if (need_update_shadowed) { + _LOGD("rollback: updating shadowed file for connection %s", + nm_connection_get_uuid(dev_checkpoint->settings_connection)); + nm_settings_connection_update( + connection, + NULL, + dev_checkpoint->settings_connection_shadowed, + NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, + sett_flags, + sett_mask, + NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS + | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET, + "checkpoint-rollback", + NULL); + } + if (need_update) { - _LOGD("rollback: updating connection %s", nm_settings_connection_get_uuid(connection)); - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP; + _LOGD("rollback: updating connection %s with persist mode \"%s\"", + nm_connection_get_uuid(dev_checkpoint->settings_connection), + nm_settings_connection_persist_mode_to_string(persist_mode)); nm_settings_connection_update( connection, NULL, @@ -246,21 +349,54 @@ restore_and_activate_connection(NMCheckpoint *self, DeviceCheckpoint *dev_checkp } } else { /* The connection was deleted, recreate it */ - _LOGD("rollback: adding connection %s again", - nm_connection_get_uuid(dev_checkpoint->settings_connection)); - - persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK; - if (!nm_settings_add_connection(NM_SETTINGS_GET, - NULL, - dev_checkpoint->settings_connection, - persist_mode, - NM_SETTINGS_CONNECTION_ADD_REASON_NONE, - sett_flags, - &connection, - &local_error)) { - _LOGD("rollback: connection add failure: %s", local_error->message); - g_clear_error(&local_error); - return FALSE; + if (need_update_shadowed) { + _LOGD("rollback: adding back shadowed file for connection %s", + nm_connection_get_uuid(dev_checkpoint->settings_connection)); + + if (!nm_settings_add_connection(NM_SETTINGS_GET, + NULL, + dev_checkpoint->settings_connection_shadowed, + NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK, + NM_SETTINGS_CONNECTION_ADD_REASON_NONE, + sett_flags, + &connection, + &local_error)) { + _LOGD("rollback: connection add failure: %s", local_error->message); + g_clear_error(&local_error); + return FALSE; + } + + _LOGD("rollback: updating connection %s with persist mode \"%s\"", + nm_connection_get_uuid(dev_checkpoint->settings_connection), + nm_settings_connection_persist_mode_to_string(persist_mode)); + + nm_settings_connection_update( + connection, + NULL, + dev_checkpoint->settings_connection, + persist_mode, + sett_flags, + sett_mask, + NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS + | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET, + "checkpoint-rollback", + NULL); + } else { + _LOGD("rollback: adding back connection %s with persist mode \"%s\"", + nm_connection_get_uuid(dev_checkpoint->settings_connection), + nm_settings_connection_persist_mode_to_string(persist_mode)); + if (!nm_settings_add_connection(NM_SETTINGS_GET, + NULL, + dev_checkpoint->settings_connection, + persist_mode, + NM_SETTINGS_CONNECTION_ADD_REASON_NONE, + sett_flags, + &connection, + &local_error)) { + _LOGD("rollback: connection add failure: %s", local_error->message); + g_clear_error(&local_error); + return FALSE; + } } need_activation = TRUE; } @@ -362,11 +498,15 @@ nm_checkpoint_rollback(NMCheckpoint *self) while (g_hash_table_iter_next(&iter, (gpointer *) &device, (gpointer *) &dev_checkpoint)) { guint32 result = NM_ROLLBACK_RESULT_OK; - _LOGD("rollback: restoring device %s (state %d, realized %d, explicitly unmanaged %d)", + _LOGD("rollback: restoring device %s (state %d, realized %d, explicitly unmanaged %d, " + "connection-unsaved %d, connection-shadowed %d, connection-shadowed-owned %d)", dev_checkpoint->original_dev_name, (int) dev_checkpoint->state, dev_checkpoint->realized, - dev_checkpoint->unmanaged_explicit); + dev_checkpoint->unmanaged_explicit, + dev_checkpoint->settings_connection_is_unsaved, + !!dev_checkpoint->settings_connection_shadowed, + dev_checkpoint->settings_connection_is_shadowed_owned); if (nm_device_is_real(device)) { if (!dev_checkpoint->realized) { @@ -518,6 +658,7 @@ device_checkpoint_destroy(gpointer data) g_clear_object(&dev_checkpoint->applied_connection); g_clear_object(&dev_checkpoint->settings_connection); g_clear_object(&dev_checkpoint->device); + g_clear_object(&dev_checkpoint->settings_connection_shadowed); g_free(dev_checkpoint->original_dev_path); g_free(dev_checkpoint->original_dev_name); @@ -555,7 +696,7 @@ _dev_exported_changed(NMDBusObject *obj, NMCheckpoint *checkpoint) } static DeviceCheckpoint * -device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device) +device_checkpoint_create(NMCheckpoint *self, NMDevice *device) { DeviceCheckpoint *dev_checkpoint; NMConnection *applied_connection; @@ -579,7 +720,7 @@ device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device) dev_checkpoint->dev_exported_change_id = g_signal_connect(device, NM_DBUS_OBJECT_EXPORTED_CHANGED, G_CALLBACK(_dev_exported_changed), - checkpoint); + self); if (nm_device_get_unmanaged_mask(device, NM_UNMANAGED_USER_EXPLICIT)) { dev_checkpoint->unmanaged_explicit = @@ -589,6 +730,11 @@ device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device) act_request = nm_device_get_act_request(device); if (act_request) { + NMSettingsStorage *storage; + gboolean shadowed_owned = FALSE; + const char *shadowed_file; + gs_free_error GError *error = NULL; + settings_connection = nm_act_request_get_settings_connection(act_request); applied_connection = nm_act_request_get_applied_connection(act_request); @@ -602,6 +748,24 @@ device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device) dev_checkpoint->activation_lifetime_bound_to_profile_visibility = NM_FLAGS_HAS(nm_active_connection_get_state_flags(NM_ACTIVE_CONNECTION(act_request)), NM_ACTIVATION_STATE_FLAG_LIFETIME_BOUND_TO_PROFILE_VISIBILITY); + + dev_checkpoint->settings_connection_is_unsaved = + NM_FLAGS_HAS(nm_settings_connection_get_flags(settings_connection), + NM_SETTINGS_CONNECTION_INT_FLAGS_UNSAVED); + + storage = nm_settings_connection_get_storage(settings_connection); + shadowed_file = + storage ? nm_settings_storage_get_shadowed_storage(storage, &shadowed_owned) : NULL; + if (shadowed_file) { + dev_checkpoint->settings_connection_is_shadowed_owned = shadowed_owned; + dev_checkpoint->settings_connection_shadowed = + parse_connection_from_shadowed_file(shadowed_file, &error); + if (!dev_checkpoint->settings_connection_shadowed) { + _LOGW("error reading shadowed connection file for %s: %s", + nm_device_get_iface(device), + error->message); + } + } } return dev_checkpoint; -- 2.41.0