From e883ae765f3eebf52fcbf73dc68a913a5ca34314 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 22 Jun 2023 11:40:42 +0200 Subject: [PATCH] settings: add "version-id" argument to Update2() D-Bus call We want to guard against concurrent modifications of profiles. We cannot lock profiles, so what we instead do is expose (and bump) a version ID. The user can check the version ID, plan ahead what to do, and tell NetworkManager to only make the modification if no concurrent modification was done. The conflict can be detected via the version ID. The Update2() D-Bus call gets a parameter to only allow the request if the version ID still matches. nmcli should use this, but it is quite some effort to retry upon concurrent modification. This is still to do. Note that the user might make a decision that is based on multiple profiles. As the new version-id is only per-profile, we cannot guard against such inter-profile modifications. What would be needed, is a UpdateMany() call, where we could modify multiple profiles at once, and the action only takes effect if all version IDs show no concurrent modification. That's not done yet, and maybe never will be. --- ...top.NetworkManager.Settings.Connection.xml | 4 ++++ src/core/settings/nm-settings-connection.c | 24 ++++++++++++++++++- src/libnm-core-public/nm-errors.h | 4 ++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/introspection/org.freedesktop.NetworkManager.Settings.Connection.xml b/introspection/org.freedesktop.NetworkManager.Settings.Connection.xml index a9240ec10..9b876e75d 100644 --- a/introspection/org.freedesktop.NetworkManager.Settings.Connection.xml +++ b/introspection/org.freedesktop.NetworkManager.Settings.Connection.xml @@ -164,6 +164,10 @@ The settings plugin the connection will be migrated to such as "keyfile" or "ifcfg-rh". Since 1.38 + version-id: + If specified, the update request is rejected if the + profile's version-id does not match. This can be used to catch concurrent + modifications. Zero means no version check.Since 1.44 diff --git a/src/core/settings/nm-settings-connection.c b/src/core/settings/nm-settings-connection.c index d0725a704..32911f16e 100644 --- a/src/core/settings/nm-settings-connection.c +++ b/src/core/settings/nm-settings-connection.c @@ -1443,6 +1443,7 @@ typedef struct { NMSettingsUpdate2Flags flags; char *audit_args; char *plugin_name; + guint64 version_id; bool is_update2 : 1; } UpdateInfo; @@ -1492,6 +1493,16 @@ update_auth_cb(NMSettingsConnection *self, priv = NM_SETTINGS_CONNECTION_GET_PRIVATE(self); + if (info->version_id != 0 && info->version_id != priv->version_id) { + g_set_error_literal(&local, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_VERSION_ID_MISMATCH, + "Update failed because profile changed in the meantime and the " + "version-id mismatches"); + error = local; + goto out; + } + if (info->new_settings) { if (!_nm_connection_aggregate(info->new_settings, NM_CONNECTION_AGGREGATE_ANY_SECRETS, @@ -1639,6 +1650,7 @@ settings_connection_update(NMSettingsConnection *self, GDBusMethodInvocation *context, GVariant *new_settings, const char *plugin_name, + guint64 version_id, NMSettingsUpdate2Flags flags) { NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE(self); @@ -1695,6 +1707,7 @@ settings_connection_update(NMSettingsConnection *self, .flags = flags, .new_settings = tmp, .plugin_name = g_strdup(plugin_name), + .version_id = version_id, }; permission = get_update_modify_permission(nm_settings_connection_get_connection(self), @@ -1729,6 +1742,7 @@ impl_settings_connection_update(NMDBusObject *obj, invocation, settings, NULL, + 0, NM_SETTINGS_UPDATE2_FLAG_TO_DISK); } @@ -1750,6 +1764,7 @@ impl_settings_connection_update_unsaved(NMDBusObject *obj, invocation, settings, NULL, + 0, NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY); } @@ -1769,6 +1784,7 @@ impl_settings_connection_save(NMDBusObject *obj, invocation, NULL, NULL, + 0, NM_SETTINGS_UPDATE2_FLAG_TO_DISK); } @@ -1785,6 +1801,7 @@ impl_settings_connection_update2(NMDBusObject *obj, gs_unref_variant GVariant *settings = NULL; gs_unref_variant GVariant *args = NULL; gs_free char *plugin_name = NULL; + guint64 version_id = 0; guint32 flags_u; GError *error = NULL; GVariantIter iter; @@ -1831,6 +1848,11 @@ impl_settings_connection_update2(NMDBusObject *obj, plugin_name = g_variant_dup_string(args_value, NULL); continue; } + if (nm_streq(args_name, "version-id") + && g_variant_is_of_type(args_value, G_VARIANT_TYPE_UINT64)) { + version_id = g_variant_get_uint64(args_value); + continue; + } error = g_error_new(NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_ARGUMENTS, @@ -1840,7 +1862,7 @@ impl_settings_connection_update2(NMDBusObject *obj, return; } - settings_connection_update(self, TRUE, invocation, settings, plugin_name, flags); + settings_connection_update(self, TRUE, invocation, settings, plugin_name, version_id, flags); } static void diff --git a/src/libnm-core-public/nm-errors.h b/src/libnm-core-public/nm-errors.h index 639c508e3..a4d69c31a 100644 --- a/src/libnm-core-public/nm-errors.h +++ b/src/libnm-core-public/nm-errors.h @@ -250,6 +250,9 @@ GQuark nm_secret_agent_error_quark(void); * @NM_SETTINGS_ERROR_UUID_EXISTS: a connection with that UUID already exists * @NM_SETTINGS_ERROR_INVALID_HOSTNAME: attempted to set an invalid hostname * @NM_SETTINGS_ERROR_INVALID_ARGUMENTS: invalid arguments + * @NM_SETTINGS_ERROR_VERSION_ID_MISMATCH: The profile's VersionId mismatched + * and the update is rejected. See the "version-id" argument to Update2() + * method. Since 1.44. * * Errors related to the settings/persistent configuration interface of * NetworkManager. @@ -267,6 +270,7 @@ typedef enum { NM_SETTINGS_ERROR_UUID_EXISTS, /*< nick=UuidExists >*/ NM_SETTINGS_ERROR_INVALID_HOSTNAME, /*< nick=InvalidHostname >*/ NM_SETTINGS_ERROR_INVALID_ARGUMENTS, /*< nick=InvalidArguments >*/ + NM_SETTINGS_ERROR_VERSION_ID_MISMATCH, /*< nick=VersionIdMismatch >*/ } NMSettingsError; GQuark nm_settings_error_quark(void);