From 88188e1ff1ffa2a4a41c9b8ee127f75cc03bc18d Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 14 May 2024 18:33:32 +0800 Subject: [PATCH 1/3] core/mount: stop generating mount units for cred mounts While @poettering wants to keep mount units for credential mounts, this has brought nothing but pain in real life. By generating mount units for each cred mount, we had trouble with default dependencies on them, which causes their stop jobs to race with unmounting through exec_context_destroy_credentials(). There were several attempts to workaround the problem, but none seems very graceful: #26959, #28787, #28957, #31360, #32011. Also, we want to carry over credentials for services that survive soft-reboot to the new mount tree, and during the practice the stop of mount units are irritating. The mentioned problems are ultimately resolved by disabling default deps: #32799. But after doing that, maybe the next question should be "why do we generate these mount units at all?" Let's revisit the whole concept here. First of all, the credential dirs are supposed to be opaque to users, and hence nobody should really reference to these mounts directly. Secondly, the lifetime of credentials is strictly bound to the service units, but nothing else. Moreover, as more and more users of credentials pop up, we could end up with hundreds of such mount units, which is something we handle poorly. And we emit useless UnitRemoved signals, etc... As discussed, it seems that eliminating these mount units is the correct way to go. No real use cases are impacted, and the lifetime management becomes sane again. Replaces #32011 --- src/core/mount.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index ebafcafa92..238e3eab50 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -425,16 +425,16 @@ static bool mount_is_extrinsic(Unit *u) { return false; } -static bool mount_is_credentials(Mount *m) { +static bool mount_point_is_credentials(Manager *manager, const char *path) { const char *e; - assert(m); + assert(manager); + assert(path); - /* Returns true if this is a credentials mount. We don't want automatic dependencies on credential - * mounts, since they are managed by us for even the earliest services, and we never want anything to - * be ordered before them hence. */ + /* Returns true if this is a credentials mount. We don't want to generate mount units for them, + * since their lifetime is strictly bound to services. */ - e = path_startswith(m->where, UNIT(m)->manager->prefix[EXEC_DIRECTORY_RUNTIME]); + e = path_startswith(path, manager->prefix[EXEC_DIRECTORY_RUNTIME]); if (!e) return false; @@ -490,7 +490,7 @@ static int mount_add_default_ordering_dependencies(Mount *m, MountParameters *p, return r; /* If this is a tmpfs mount then we have to unmount it before we try to deactivate swaps */ - if (streq_ptr(p->fstype, "tmpfs") && !mount_is_credentials(m)) { + if (streq_ptr(p->fstype, "tmpfs")) { r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_SWAP_TARGET, /* add_reference= */ true, mask); if (r < 0) @@ -578,12 +578,16 @@ static int mount_verify(Mount *m) { return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), "Where= setting doesn't match unit name. Refusing."); if (mount_point_is_api(m->where) || mount_point_ignore(m->where)) - return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), "Cannot create mount unit for API file system %s. Refusing.", m->where); + return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), + "Cannot create mount unit for API file system '%s'. Refusing.", m->where); + + if (mount_point_is_credentials(UNIT(m)->manager, m->where)) + return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), + "Cannot create mount unit for credential mount '%s'. Refusing.", m->where); p = get_mount_parameters_fragment(m); if (p && !p->what && !UNIT(m)->perpetual) - return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), - "What= setting is missing. Refusing."); + return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), "What= setting is missing. Refusing."); if (m->exec_context.pam_name && m->kill_context.kill_mode != KILL_CONTROL_GROUP) return log_unit_error_errno(UNIT(m), SYNTHETIC_ERRNO(ENOEXEC), "Unit has PAM enabled. Kill mode must be set to control-group'. Refusing."); @@ -604,9 +608,6 @@ static int mount_add_non_exec_dependencies(Mount *m) { if (!m->where) return 0; - if (mount_is_credentials(m)) - UNIT(m)->default_dependencies = false; - /* Adds in all dependencies directly responsible for ordering the mount, as opposed to dependencies * resulting from the ExecContext and such. */ @@ -1840,9 +1841,9 @@ static int mount_setup_unit( assert(options); assert(fstype); - /* Ignore API mount points. They should never be referenced in - * dependencies ever. */ - if (mount_point_is_api(where) || mount_point_ignore(where)) + /* Ignore API and credential mount points. They should never be referenced in dependencies ever. + * Also check the comment for mount_point_is_credentials. */ + if (mount_point_is_api(where) || mount_point_ignore(where) || mount_point_is_credentials(m, where)) return 0; if (streq(fstype, "autofs")) From c5087ef51099521450eb3486c606c99d86a1a009 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 14 May 2024 18:47:42 +0800 Subject: [PATCH 2/3] Revert "core/credential,mount: re-read /proc/self/mountinfo before invoking umount command" This reverts commit 1e1225614ca1106116dcad9fb37aaeb6106408ab. This was an incomplete workaround of the race. Now that we stop generating mount units for credential mounts, the logic could be dropped. --- src/core/exec-credential.c | 11 ++++------- src/core/exec-credential.h | 2 +- src/core/mount.c | 32 -------------------------------- src/core/mount.h | 4 ---- src/core/unit.c | 2 +- 5 files changed, 6 insertions(+), 45 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index f4cff57848..d58aa18b55 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -13,7 +13,6 @@ #include "label-util.h" #include "mkdir-label.h" #include "mount-util.h" -#include "mount.h" #include "mountpoint-util.h" #include "process-util.h" #include "random-util.h" @@ -145,21 +144,19 @@ int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c) { return unit_add_dependency_by_name(u, UNIT_AFTER, m, /* add_reference= */ true, UNIT_DEPENDENCY_FILE); } -int exec_context_destroy_credentials(Unit *u) { +int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_prefix, const char *unit) { _cleanup_free_ char *p = NULL; int r; - assert(u); + assert(c); - r = get_credential_directory(u->manager->prefix[EXEC_DIRECTORY_RUNTIME], u->id, &p); + r = get_credential_directory(runtime_prefix, unit, &p); if (r <= 0) return r; /* This is either a tmpfs/ramfs of its own, or a plain directory. Either way, let's first try to * unmount it, and afterwards remove the mount point */ - if (umount2(p, MNT_DETACH|UMOUNT_NOFOLLOW) >= 0) - (void) mount_invalidate_state_by_path(u->manager, p); - + (void) umount2(p, MNT_DETACH|UMOUNT_NOFOLLOW); (void) rm_rf(p, REMOVE_ROOT|REMOVE_CHMOD); return 0; diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h index 70bb46bdde..f02770cec5 100644 --- a/src/core/exec-credential.h +++ b/src/core/exec-credential.h @@ -47,7 +47,7 @@ int exec_context_get_credential_directory( int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c); -int exec_context_destroy_credentials(Unit *u); +int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_root, const char *unit); int exec_setup_credentials( const ExecContext *context, const ExecParameters *params, diff --git a/src/core/mount.c b/src/core/mount.c index 238e3eab50..a6460287ce 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1333,11 +1333,6 @@ static int mount_start(Unit *u) { static int mount_stop(Unit *u) { Mount *m = ASSERT_PTR(MOUNT(u)); - /* When we directly call umount() for a path, then the state of the corresponding mount unit may be - * outdated. Let's re-read mountinfo now and update the state. */ - if (m->invalidated_state) - (void) mount_process_proc_self_mountinfo(u->manager); - switch (m->state) { case MOUNT_UNMOUNTING: @@ -1372,11 +1367,6 @@ static int mount_stop(Unit *u) { mount_enter_signal(m, MOUNT_UNMOUNTING_SIGKILL, MOUNT_SUCCESS); return 0; - case MOUNT_DEAD: - case MOUNT_FAILED: - /* The mount has just been unmounted by somebody else. */ - return 0; - default: assert_not_reached(); } @@ -2141,8 +2131,6 @@ static int mount_process_proc_self_mountinfo(Manager *m) { LIST_FOREACH(units_by_type, u, m->units_by_type[UNIT_MOUNT]) { Mount *mount = MOUNT(u); - mount->invalidated_state = false; - if (!mount_is_mounted(mount)) { /* A mount point is not around right now. It might be gone, or might never have @@ -2239,26 +2227,6 @@ static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents, return mount_process_proc_self_mountinfo(m); } -int mount_invalidate_state_by_path(Manager *manager, const char *path) { - _cleanup_free_ char *name = NULL; - Unit *u; - int r; - - assert(manager); - assert(path); - - r = unit_name_from_path(path, ".mount", &name); - if (r < 0) - return log_debug_errno(r, "Failed to generate unit name from path \"%s\", ignoring: %m", path); - - u = manager_get_unit(manager, name); - if (!u) - return -ENOENT; - - MOUNT(u)->invalidated_state = true; - return 0; -} - static void mount_reset_failed(Unit *u) { Mount *m = MOUNT(u); diff --git a/src/core/mount.h b/src/core/mount.h index a029dc87ac..9a8b6bb4c0 100644 --- a/src/core/mount.h +++ b/src/core/mount.h @@ -50,8 +50,6 @@ struct Mount { MountParameters parameters_proc_self_mountinfo; MountParameters parameters_fragment; - bool invalidated_state:1; /* Set when the 'state' of the mount unit may be outdated, and we need to - * re-read /proc/self/mountinfo. */ bool from_proc_self_mountinfo:1; bool from_fragment:1; @@ -96,8 +94,6 @@ extern const UnitVTable mount_vtable; void mount_fd_event(Manager *m, int events); -int mount_invalidate_state_by_path(Manager *manager, const char *path); - char* mount_get_what_escaped(const Mount *m); char* mount_get_options_escaped(const Mount *m); const char* mount_get_fstype(const Mount *m); diff --git a/src/core/unit.c b/src/core/unit.c index 2d40618fcb..a1e7b62b9a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -6109,7 +6109,7 @@ void unit_destroy_runtime_data(Unit *u, const ExecContext *context) { if (context->runtime_directory_preserve_mode == EXEC_PRESERVE_NO) exec_context_destroy_runtime_directory(context, u->manager->prefix[EXEC_DIRECTORY_RUNTIME]); - exec_context_destroy_credentials(u); + exec_context_destroy_credentials(context, u->manager->prefix[EXEC_DIRECTORY_RUNTIME], u->id); exec_context_destroy_mount_ns_dir(u); } From 1899715826c1617bd945b759e865a9f1b8a2db5b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 14 May 2024 21:28:33 +0800 Subject: [PATCH 3/3] core/exec-credential: drop unused unit_add_default_credential_dependencies --- src/core/exec-credential.c | 25 ------------------------- src/core/exec-credential.h | 4 +--- src/core/unit.c | 4 ---- 3 files changed, 1 insertion(+), 32 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index d58aa18b55..e46a76f0ac 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -119,31 +119,6 @@ int exec_context_get_credential_directory( return get_credential_directory(params->prefix[EXEC_DIRECTORY_RUNTIME], unit, ret); } -int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c) { - _cleanup_free_ char *p = NULL, *m = NULL; - int r; - - assert(u); - assert(c); - - if (!exec_context_has_credentials(c)) - return 0; - - /* Let's make sure the credentials directory of this service is unmounted *after* the service itself - * shuts down. This only matters if mount namespacing is not used for the service, and hence the - * credentials mount appears on the host. */ - - r = get_credential_directory(u->manager->prefix[EXEC_DIRECTORY_RUNTIME], u->id, &p); - if (r <= 0) - return r; - - r = unit_name_from_path(p, ".mount", &m); - if (r < 0) - return r; - - return unit_add_dependency_by_name(u, UNIT_AFTER, m, /* add_reference= */ true, UNIT_DEPENDENCY_FILE); -} - int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_prefix, const char *unit) { _cleanup_free_ char *p = NULL; int r; diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h index f02770cec5..509c91e829 100644 --- a/src/core/exec-credential.h +++ b/src/core/exec-credential.h @@ -9,7 +9,6 @@ typedef struct ExecContext ExecContext; typedef struct ExecParameters ExecParameters; -typedef struct Unit Unit; /* A credential configured with LoadCredential= */ typedef struct ExecLoadCredential { @@ -45,9 +44,8 @@ int exec_context_get_credential_directory( const char *unit, char **ret); -int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c); - int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_root, const char *unit); + int exec_setup_credentials( const ExecContext *context, const ExecParameters *params, diff --git a/src/core/unit.c b/src/core/unit.c index a1e7b62b9a..1d2311f035 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1337,10 +1337,6 @@ int unit_add_exec_dependencies(Unit *u, ExecContext *c) { return r; } - r = unit_add_default_credential_dependencies(u, c); - if (r < 0) - return r; - return 0; }