From 21dd1de659935c829a3776b6772f803c2eb4eae3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 3 May 2023 22:45:05 +0200 Subject: [PATCH 1/2] execute: suppress credentials mount if empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's avoid creating another mount in the system if it's empty anyway. This is mostl a cosmetic thing in one (pretty common) special case: if creds settings are used in a unit but no creds actually available to be passed. (While we are at it this also does one more minor optimization: it adjusts the MS_RDONLY/MS_NOSUID/… flags of the source mount we are about to MS_MOVE into the right place only if we actually really move it, and if we instead unmount it again we won't bother with the flags either) --- src/core/execute.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index b32e341b61..b803edb145 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3258,16 +3258,34 @@ static int setup_credentials_internal( return r; if (workspace_mounted) { - /* Make workspace read-only now, so that any bind mount we make from it defaults to read-only too */ - r = mount_nofollow_verbose(LOG_DEBUG, NULL, workspace, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY|MS_NODEV|MS_NOEXEC|MS_NOSUID, NULL); - if (r < 0) - return r; + bool install; - /* And mount it to the final place, read-only */ + /* Determine if we should actually install the prepared mount in the final location by bind + * mounting it there. We do so only if the mount is not established there already, and if the + * mount is actually non-empty (i.e. carries at least one credential). Not that in the best + * case we are doing all this in a mount namespace, thus noone else will see that we + * allocated a file system we are getting rid of again here. */ if (final_mounted) - r = umount_verbose(LOG_DEBUG, workspace, MNT_DETACH|UMOUNT_NOFOLLOW); - else + install = false; /* already installed */ + else { + r = dir_is_empty(where, /* ignore_hidden_or_backup= */ false); + if (r < 0) + return r; + + install = r == 0; /* install only if non-empty */ + } + + if (install) { + /* Make workspace read-only now, so that any bind mount we make from it defaults to read-only too */ + r = mount_nofollow_verbose(LOG_DEBUG, NULL, workspace, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY|MS_NODEV|MS_NOEXEC|MS_NOSUID, NULL); + if (r < 0) + return r; + + /* And mount it to the final place, read-only */ r = mount_nofollow_verbose(LOG_DEBUG, workspace, final, NULL, MS_MOVE, NULL); + } else + /* Otherwise get rid of it */ + r = umount_verbose(LOG_DEBUG, workspace, MNT_DETACH|UMOUNT_NOFOLLOW); if (r < 0) return r; } else { From 3aaa376342eafa5f5ab4d289a657d22be562d33f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 4 May 2023 12:06:10 +0200 Subject: [PATCH 2/2] execute: remove credentials dir again when empty This is closely related to the previous commit: if the credentials dir is empty and nothing mounted on it, let's remove it again. This will in particular happen if we decided to not actually install the mount we prepared for the credentials because it is empty. In that case the mount point inode is already there, and with this we'll remove it. Primary effect, users will see ENOENT rather than EACCESS when trying to access it, which should be preferable, given we already handle that nicely in our credential consumption code. This should also be useful on systems where we lack any privs to create mounts, and thus operate on a regular dir anyway. --- src/core/execute.c | 5 +++++ src/core/namespace.c | 1 + 2 files changed, 6 insertions(+) diff --git a/src/core/execute.c b/src/core/execute.c index b803edb145..04dcf4b427 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3423,6 +3423,11 @@ static int setup_credentials( _exit(EXIT_FAILURE); } + /* If the credentials dir is empty and not a mount point, then there's no point in having it. Let's + * try to remove it. This matters in particular if we created the dir as mount point but then didn't + * actually end up mounting anything on it. In that case we'd rather have ENOENT than EACCESS being + * seen by users when trying access this inode. */ + (void) rmdir(p); return 0; } diff --git a/src/core/namespace.c b/src/core/namespace.c index 1d19685d2e..2fcc096217 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -2385,6 +2385,7 @@ int setup_namespace( .mode = BIND_MOUNT, .read_only = true, .source_const = creds_path, + .ignore = true, }; } else { /* If our service has no credentials store configured, then make the whole