From e73c042be6ebb8ca08a32595049938b03ea10de4 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Sat, 26 Oct 2024 15:28:49 -0700 Subject: [PATCH 1/2] core/execute: Rename error_path -> reterr_path/ret_path per coding guidelines This is a non-functional change to ensure error_path used to print out the offending mount causing an error follows coding guidelines. --- src/core/exec-invoke.c | 4 ++-- src/core/namespace.c | 30 +++++++++++++++--------------- src/core/namespace.h | 2 +- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 0dbf34042d..5dfbafb282 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3040,7 +3040,7 @@ static int apply_mount_namespace( ExecRuntime *runtime, const char *memory_pressure_path, bool needs_sandboxing, - char **error_path) { + char **reterr_path) { _cleanup_(verity_settings_done) VeritySettings verity = VERITY_SETTINGS_DEFAULT; _cleanup_strv_free_ char **empty_directories = NULL, **symlinks = NULL, @@ -3249,7 +3249,7 @@ static int apply_mount_namespace( .proc_subset = needs_sandboxing ? context->proc_subset : false, }; - r = setup_namespace(¶meters, error_path); + r = setup_namespace(¶meters, reterr_path); /* If we couldn't set up the namespace this is probably due to a missing capability. setup_namespace() reports * that with a special, recognizable error ENOANO. In this case, silently proceed, but only if exclusively * sandboxing options were used, i.e. nothing such as RootDirectory= or BindMount= that would result in a diff --git a/src/core/namespace.c b/src/core/namespace.c index ba308c2957..c45be457a8 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -1990,7 +1990,7 @@ static int create_symlinks_from_tuples(const char *root, char **strv_symlinks) { return 0; } -static void mount_entry_path_debug_string(const char *root, MountEntry *m, char **error_path) { +static void mount_entry_path_debug_string(const char *root, MountEntry *m, char **ret_path) { assert(m); /* Create a string suitable for debugging logs, stripping for example the local working directory. @@ -2003,23 +2003,23 @@ static void mount_entry_path_debug_string(const char *root, MountEntry *m, char * * Note that this is an error path, so no OOM check is done on purpose. */ - if (!error_path) + if (!ret_path) return; if (!mount_entry_path(m)) { - *error_path = NULL; + *ret_path = NULL; return; } if (root) { const char *e = startswith(mount_entry_path(m), root); if (e) { - *error_path = strdup(e); + *ret_path = strdup(e); return; } } - *error_path = strdup(mount_entry_path(m)); + *ret_path = strdup(mount_entry_path(m)); return; } @@ -2027,7 +2027,7 @@ static int apply_mounts( MountList *ml, const char *root, const NamespaceParameters *p, - char **error_path) { + char **reterr_path) { _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; _cleanup_free_ char **deny_list = NULL; @@ -2046,8 +2046,8 @@ static int apply_mounts( if (!proc_self_mountinfo) { r = -errno; - if (error_path) - *error_path = strdup("/proc/self/mountinfo"); + if (reterr_path) + *reterr_path = strdup("/proc/self/mountinfo"); return log_debug_errno(r, "Failed to open /proc/self/mountinfo: %m"); } @@ -2067,7 +2067,7 @@ static int apply_mounts( * /tmp and /var/tmp. */ r = follow_symlink(!IN_SET(m->mode, MOUNT_EXTENSION_IMAGE, MOUNT_EXTENSION_DIRECTORY, MOUNT_PRIVATE_TMPFS) ? root : NULL, m); if (r < 0) { - mount_entry_path_debug_string(root, m, error_path); + mount_entry_path_debug_string(root, m, reterr_path); return r; } if (r == 0) { @@ -2082,7 +2082,7 @@ static int apply_mounts( /* Returns 1 if the mount should be post-processed, 0 otherwise */ r = apply_one_mount(root, m, p); if (r < 0) { - mount_entry_path_debug_string(root, m, error_path); + mount_entry_path_debug_string(root, m, reterr_path); return r; } m->state = r == 0 ? MOUNT_SKIPPED : MOUNT_APPLIED; @@ -2114,7 +2114,7 @@ static int apply_mounts( FOREACH_ARRAY(m, ml->mounts, ml->n_mounts) { r = make_read_only(m, deny_list, proc_self_mountinfo); if (r < 0) { - mount_entry_path_debug_string(root, m, error_path); + mount_entry_path_debug_string(root, m, reterr_path); return r; } } @@ -2128,7 +2128,7 @@ static int apply_mounts( FOREACH_ARRAY(m, ml->mounts, ml->n_mounts) { r = make_noexec(m, deny_list, proc_self_mountinfo); if (r < 0) { - mount_entry_path_debug_string(root, m, error_path); + mount_entry_path_debug_string(root, m, reterr_path); return r; } } @@ -2138,7 +2138,7 @@ static int apply_mounts( FOREACH_ARRAY(m, ml->mounts, ml->n_mounts) { r = make_nosuid(m, proc_self_mountinfo); if (r < 0) { - mount_entry_path_debug_string(root, m, error_path); + mount_entry_path_debug_string(root, m, reterr_path); return r; } } @@ -2195,7 +2195,7 @@ static bool home_read_only( return false; } -int setup_namespace(const NamespaceParameters *p, char **error_path) { +int setup_namespace(const NamespaceParameters *p, char **reterr_path) { _cleanup_(loop_device_unrefp) LoopDevice *loop_device = NULL; _cleanup_(dissected_image_unrefp) DissectedImage *dissected_image = NULL; @@ -2747,7 +2747,7 @@ int setup_namespace(const NamespaceParameters *p, char **error_path) { (void) base_filesystem_create(root, UID_INVALID, GID_INVALID); /* Now make the magic happen */ - r = apply_mounts(&ml, root, p, error_path); + r = apply_mounts(&ml, root, p, reterr_path); if (r < 0) return r; diff --git a/src/core/namespace.h b/src/core/namespace.h index dfb42b7c37..2c8f7987fe 100644 --- a/src/core/namespace.h +++ b/src/core/namespace.h @@ -172,7 +172,7 @@ struct NamespaceParameters { PrivateTmp private_tmp; }; -int setup_namespace(const NamespaceParameters *p, char **error_path); +int setup_namespace(const NamespaceParameters *p, char **reterr_path); #define RUN_SYSTEMD_EMPTY "/run/systemd/empty" From 141dfbe537c3f5692d49df17629e5cf98ec8ec8b Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Sat, 26 Oct 2024 15:33:30 -0700 Subject: [PATCH 2/2] core: Add RootDirectory= path to error message if directory does not exist A colleague reported when RootDirectory= does not exist, systemd reports an error like: ``` Failed to set up mount namespacing: No such file or directory ``` Unfortunately, with large spec files, it can be hard to diagnose which path systemd is talking about. Thus, to make the error message more helpful and similar to mount error messages, we add the root directory/image path into the error message like: ``` Failed to set up mount namespacing: /tmp/thisdoesnotexist: No such file or directory ``` --- src/core/exec-invoke.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 5dfbafb282..c71d6b4c2a 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -2845,7 +2845,8 @@ static int setup_ephemeral( const ExecContext *context, ExecRuntime *runtime, char **root_image, /* both input and output! modified if ephemeral logic enabled */ - char **root_directory) { /* ditto */ + char **root_directory, /* ditto */ + char **reterr_path) { _cleanup_close_ int fd = -EBADF; _cleanup_free_ char *new_root = NULL; @@ -2886,9 +2887,11 @@ static int setup_ephemeral( fd = copy_file(*root_image, new_root, O_EXCL, 0600, COPY_LOCK_BSD|COPY_REFLINK|COPY_CRTIME|COPY_NOCOW_AFTER); - if (fd < 0) + if (fd < 0) { + *reterr_path = strdup(*root_image); return log_debug_errno(fd, "Failed to copy image %s to %s: %m", *root_image, new_root); + } } else { assert(*root_directory); @@ -2901,9 +2904,11 @@ static int setup_ephemeral( BTRFS_SNAPSHOT_FALLBACK_DIRECTORY | BTRFS_SNAPSHOT_RECURSIVE | BTRFS_SNAPSHOT_LOCK_BSD); - if (fd < 0) + if (fd < 0) { + *reterr_path = strdup(*root_directory); return log_debug_errno(fd, "Failed to snapshot directory %s to %s: %m", *root_directory, new_root); + } } r = send_one_fd(runtime->ephemeral_storage_socket[1], fd, MSG_DONTWAIT); @@ -2980,7 +2985,8 @@ static int pick_versions( const ExecContext *context, const ExecParameters *params, char **ret_root_image, - char **ret_root_directory) { + char **ret_root_directory, + char **reterr_path) { int r; @@ -2998,11 +3004,15 @@ static int pick_versions( &pick_filter_image_raw, PICK_ARCHITECTURE|PICK_TRIES|PICK_RESOLVE, &result); - if (r < 0) + if (r < 0) { + *reterr_path = strdup(context->root_image); return r; + } - if (!result.path) + if (!result.path) { + *reterr_path = strdup(context->root_image); return log_exec_debug_errno(context, params, SYNTHETIC_ERRNO(ENOENT), "No matching entry in .v/ directory %s found.", context->root_image); + } *ret_root_image = TAKE_PTR(result.path); *ret_root_directory = NULL; @@ -3018,11 +3028,15 @@ static int pick_versions( &pick_filter_image_dir, PICK_ARCHITECTURE|PICK_TRIES|PICK_RESOLVE, &result); - if (r < 0) + if (r < 0) { + *reterr_path = strdup(context->root_directory); return r; + } - if (!result.path) + if (!result.path) { + *reterr_path = strdup(context->root_directory); return log_exec_debug_errno(context, params, SYNTHETIC_ERRNO(ENOENT), "No matching entry in .v/ directory %s found.", context->root_directory); + } *ret_root_image = NULL; *ret_root_directory = TAKE_PTR(result.path); @@ -3063,7 +3077,8 @@ static int apply_mount_namespace( context, params, &root_image, - &root_dir); + &root_dir, + reterr_path); if (r < 0) return r; @@ -3071,7 +3086,8 @@ static int apply_mount_namespace( context, runtime, &root_image, - &root_dir); + &root_dir, + reterr_path); if (r < 0) return r; }