From b97936807556203d899ba6225acf96b0557b52b8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 16 Jan 2025 10:17:44 +0100 Subject: [PATCH 01/11] mount-util: introduce umountat_detach_verbose() This new helper allows unmounting paths by fd. --- src/shared/mount-util.c | 58 ++++++++++++++++++++++++++++++++++---- src/shared/mount-util.h | 5 ++++ src/test/test-mount-util.c | 20 +++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 39aa3d3ec8..a9ce8d7e5a 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -736,16 +736,62 @@ int mount_verbose_full( int umount_verbose( int error_log_level, - const char *what, + const char *where, int flags) { - assert(what); + assert(where); - log_debug("Umounting %s...", what); + log_debug("Unmounting '%s'...", where); - if (umount2(what, flags) < 0) - return log_full_errno(error_log_level, errno, - "Failed to unmount %s: %m", what); + if (umount2(where, flags) < 0) + return log_full_errno(error_log_level, errno, "Failed to unmount '%s': %m", where); + + return 0; +} + +int umountat_detach_verbose( + int error_log_level, + int fd, + const char *where) { + + /* Similar to umountat_verbose(), but goes by fd + path. This implies MNT_DETACH, since to do this we + * must pin the inode in question via an fd. */ + + assert(fd >= 0 || fd == AT_FDCWD); + + /* If neither fd nor path are specified take this as reference to the cwd */ + if (fd == AT_FDCWD && isempty(where)) + return umount_verbose(error_log_level, ".", MNT_DETACH|UMOUNT_NOFOLLOW); + + /* If we don't actually take the fd into consideration for this operation shortcut things, so that we + * don't have to open the inode */ + if (fd == AT_FDCWD || path_is_absolute(where)) + return umount_verbose(error_log_level, where, MNT_DETACH|UMOUNT_NOFOLLOW); + + _cleanup_free_ char *prefix = NULL; + const char *p; + if (fd_get_path(fd, &prefix) < 0) + p = ""; /* if we can't get the path, return something vaguely useful */ + else + p = prefix; + _cleanup_free_ char *joined = isempty(where) ? strdup(p) : path_join(p, where); + + log_debug("Unmounting '%s'...", strna(joined)); + + _cleanup_close_ int inode_fd = -EBADF; + int mnt_fd; + if (isempty(where)) + mnt_fd = fd; + else { + inode_fd = openat(fd, where, O_PATH|O_CLOEXEC|O_NOFOLLOW); + if (inode_fd < 0) + return log_full_errno(error_log_level, errno, "Failed to pin '%s': %m", strna(joined)); + + mnt_fd = inode_fd; + } + + if (umount2(FORMAT_PROC_FD_PATH(mnt_fd), MNT_DETACH) < 0) + return log_full_errno(error_log_level, errno, "Failed to unmount '%s': %m", strna(joined)); return 0; } diff --git a/src/shared/mount-util.h b/src/shared/mount-util.h index 496a95ab05..3235a3cc81 100644 --- a/src/shared/mount-util.h +++ b/src/shared/mount-util.h @@ -79,6 +79,11 @@ int umount_verbose( const char *where, int flags); +int umountat_detach_verbose( + int error_log_level, + int fd, + const char *where); + int mount_option_mangle( const char *options, unsigned long mount_flags, diff --git a/src/test/test-mount-util.c b/src/test/test-mount-util.c index 4ac8f869d6..0d7e803b54 100644 --- a/src/test/test-mount-util.c +++ b/src/test/test-mount-util.c @@ -587,4 +587,24 @@ TEST(path_is_network_fs_harder) { } } +TEST(umountat) { + int r; + + _cleanup_(rm_rf_physical_and_freep) char *p = NULL; + _cleanup_close_ int dfd = mkdtemp_open(NULL, O_CLOEXEC, &p); + ASSERT_OK(dfd); + + ASSERT_OK(mkdirat(dfd, "foo", 0777)); + + _cleanup_free_ char *q = ASSERT_PTR(path_join(p, "foo")); + + r = mount_nofollow_verbose(LOG_ERR, "tmpfs", q, "tmpfs", 0, NULL); + if (ERRNO_IS_NEG_PRIVILEGE(r)) + return (void) log_tests_skipped("not running privileged"); + + ASSERT_OK(r); + ASSERT_OK(umountat_detach_verbose(LOG_ERR, dfd, "foo")); + ASSERT_ERROR(umountat_detach_verbose(LOG_ERR, dfd, "foo"), EINVAL); +} + DEFINE_TEST_MAIN(LOG_DEBUG); From e49e76d64df09bea3c54385f0cf0161b61466ec6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2025 12:53:54 +0100 Subject: [PATCH 02/11] chase: allow using chase() as mkdir_p() replacement This allows using CHASE_MKDIR_0755 without CHASE_NONEXISTENT or CHASE_PARENT, so that it will create the final component of the path too should it be missing. This is really useful as a mkdir_p() replacement that returns an fd to the final component, and knows how to operate relative to a root fs. Kinda reverts 4ea0bcb9229fe12e0c428659d76934351b821872 (which only refused the flags combination which didn't work, instead of making it work, which is what this commit does.) This also corrects behaviour if CHASE_MKDIR_0755 is used in one more way: we'll now always open the dir as O_PATH. This is generally the better idea, but matters in particular once with allow using CHASE_MKDIR_0755 to create the final component: we should uniformly return an O_PATH dir that must be converted to a proper fd first before using it. --- src/basic/chase.c | 7 +++---- src/basic/chase.h | 6 +----- src/test/test-chase.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 43fad0d93f..b1e27b48c1 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -91,7 +91,6 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int assert(!FLAGS_SET(flags, CHASE_PREFIX_ROOT)); assert(!FLAGS_SET(flags, CHASE_STEP|CHASE_EXTRACT_FILENAME)); assert(!FLAGS_SET(flags, CHASE_TRAIL_SLASH|CHASE_EXTRACT_FILENAME)); - assert(!FLAGS_SET(flags, CHASE_MKDIR_0755) || (flags & (CHASE_NONEXISTENT | CHASE_PARENT)) != 0); assert(dir_fd >= 0 || dir_fd == AT_FDCWD); /* Either the file may be missing, or we return an fd to the final object, but both make no sense */ @@ -370,13 +369,13 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (r != -ENOENT) return r; - if (!isempty(todo) && !path_is_safe(todo)) + if (!isempty(todo) && !path_is_safe(todo)) /* Refuse parent/mkdir handling if suffix contains ".." or something weird */ return r; - if (FLAGS_SET(flags, CHASE_MKDIR_0755) && !isempty(todo)) { + if (FLAGS_SET(flags, CHASE_MKDIR_0755) && (!isempty(todo) || !(flags & (CHASE_PARENT|CHASE_NONEXISTENT)))) { child = xopenat_full(fd, first, - O_DIRECTORY|O_CREAT|O_EXCL|O_NOFOLLOW|O_CLOEXEC, + O_DIRECTORY|O_CREAT|O_EXCL|O_NOFOLLOW|O_PATH|O_CLOEXEC, /* xopen_flags = */ 0, 0755); if (child < 0) diff --git a/src/basic/chase.h b/src/basic/chase.h index cfc714b9f7..cedd7097fb 100644 --- a/src/basic/chase.h +++ b/src/basic/chase.h @@ -27,11 +27,7 @@ typedef enum ChaseFlags { * also points to the result path even if this flag is set. * When this specified, chase() will succeed with 1 even if the * file points to the last path component does not exist. */ - CHASE_MKDIR_0755 = 1 << 11, /* Create any missing parent directories in the given path. This - * needs to be set with CHASE_NONEXISTENT and/or CHASE_PARENT. - * Note, chase_and_open() or friends always add CHASE_PARENT flag - * when internally call chase(), hence CHASE_MKDIR_0755 can be - * safely set without CHASE_NONEXISTENT and CHASE_PARENT. */ + CHASE_MKDIR_0755 = 1 << 11, /* Create any missing directories in the given path. */ CHASE_EXTRACT_FILENAME = 1 << 12, /* Only return the last component of the resolved path */ } ChaseFlags; diff --git a/src/test/test-chase.c b/src/test/test-chase.c index c7ca3fd051..510264c547 100644 --- a/src/test/test-chase.c +++ b/src/test/test-chase.c @@ -10,6 +10,7 @@ #include "id128-util.h" #include "mkdir.h" #include "path-util.h" +#include "random-util.h" #include "rm-rf.h" #include "string-util.h" #include "tests.h" @@ -754,6 +755,34 @@ TEST(trailing_dot_dot) { assert_se(path_equal(fdpath, expected2)); } +TEST(use_chase_as_mkdir_p) { + _cleanup_free_ char *p = NULL; + ASSERT_OK_ERRNO(asprintf(&p, "/tmp/chasemkdir%" PRIu64 "/a/b/c", random_u64())); + + _cleanup_close_ int fd = -EBADF; + ASSERT_OK(chase(p, NULL, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755, NULL, &fd)); + + ASSERT_OK_EQ(inode_same_at(AT_FDCWD, p, fd, NULL, AT_EMPTY_PATH), 1); + + _cleanup_close_ int fd2 = -EBADF; + ASSERT_OK(chase(p, p, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755, NULL, &fd2)); + + _cleanup_free_ char *pp = ASSERT_PTR(path_join(p, p)); + + ASSERT_OK_EQ(inode_same_at(AT_FDCWD, pp, fd2, NULL, AT_EMPTY_PATH), 1); + + _cleanup_free_ char *f = NULL; + ASSERT_OK(path_extract_directory(p, &f)); + + _cleanup_free_ char *ff = NULL; + ASSERT_OK(path_extract_directory(f, &ff)); + + _cleanup_free_ char *fff = NULL; + ASSERT_OK(path_extract_directory(ff, &fff)); + + ASSERT_OK(rm_rf(fff, REMOVE_PHYSICAL)); +} + static int intro(void) { arg_test_dir = saved_argv[1]; return EXIT_SUCCESS; From b0afd29f7d23f518ed11ded6108a775880ef6379 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2025 12:56:57 +0100 Subject: [PATCH 03/11] chase: use streq() not path_equal() to compare filenames --- src/basic/chase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index b1e27b48c1..a0edb7e11c 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -267,7 +267,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int return -ENOMEM; /* Two dots? Then chop off the last bit of what we already found out. */ - if (path_equal(first, "..")) { + if (streq(first, "..")) { _cleanup_free_ char *parent = NULL; _cleanup_close_ int fd_parent = -EBADF; struct stat st_parent; From 849552c80bc9943ee25aaeb15cf0c26c4deef192 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 15 Jan 2025 21:58:31 +0100 Subject: [PATCH 04/11] machine-id-setup: rearrange --help to match how we do them these days --- src/machine-id-setup/machine-id-setup-main.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/machine-id-setup/machine-id-setup-main.c b/src/machine-id-setup/machine-id-setup-main.c index cfb94841b9..fa4ba6a037 100644 --- a/src/machine-id-setup/machine-id-setup-main.c +++ b/src/machine-id-setup/machine-id-setup-main.c @@ -36,18 +36,21 @@ static int help(void) { if (r < 0) return log_oom(); - printf("%s [OPTIONS...]\n" - "\n%sInitialize /etc/machine-id from a random source.%s\n\n" + printf("%1$s [OPTIONS...]\n" + "\n%2$sInitialize /etc/machine-id from a random source.%4$s\n" + "\n%3$sCommands:%4$s\n" + " --commit Commit transient ID\n" " -h --help Show this help\n" " --version Show package version\n" + "\n%3$sOptions:%4$s\n" " --root=PATH Operate on an alternate filesystem root\n" " --image=PATH Operate on disk image as filesystem root\n" " --image-policy=POLICY Specify disk image dissection policy\n" - " --commit Commit transient ID\n" " --print Print used machine ID\n" - "\nSee the %s for details.\n", + "\nSee the %5$s for details.\n", program_invocation_short_name, ansi_highlight(), + ansi_underline(), ansi_normal(), link); From 4d0fb50e9ea2c3caa167e744145aef5cf24efb9e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 16 Jan 2025 10:22:39 +0100 Subject: [PATCH 05/11] machine-id-setup: do sync() stuff only when operating on the real rootfs If we operate relative some explicit rootfs the reason for the sync()ing is not really given, we are not really acting as a milestone at boot, but "offline". --- src/shared/machine-id-setup.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/shared/machine-id-setup.c b/src/shared/machine-id-setup.c index 4240525742..5526a900f4 100644 --- a/src/shared/machine-id-setup.c +++ b/src/shared/machine-id-setup.c @@ -255,17 +255,20 @@ int machine_id_commit(const char *root) { sd_id128_t id; int r; - /* Before doing anything, sync everything to ensure any changes by first-boot units are persisted. - * - * First, explicitly sync the file systems we care about and check if it worked. */ - FOREACH_STRING(sync_path, "/etc/", "/var/") { - r = syncfs_path(AT_FDCWD, sync_path); - if (r < 0) - return log_error_errno(r, "Cannot sync %s: %m", sync_path); - } + if (empty_or_root(root)) { + /* Before doing anything, sync everything to ensure any changes by first-boot units are + * persisted. + * + * First, explicitly sync the file systems we care about and check if it worked. */ + FOREACH_STRING(sync_path, "/etc/", "/var/") { + r = syncfs_path(AT_FDCWD, sync_path); + if (r < 0) + return log_error_errno(r, "Cannot sync %s: %m", sync_path); + } - /* Afterwards, sync() the rest too, but we can't check the return value for these. */ - sync(); + /* Afterwards, sync() the rest too, but we can't check the return value for these. */ + sync(); + } /* Replaces a tmpfs bind mount of /etc/machine-id by a proper file, atomically. For this, the umount is removed * in a mount namespace, a new file is created at the right place. Afterwards the mount is also removed in the From efabf4e04466c85183c969af27de1ff0ecda8e79 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 12 Jan 2025 00:17:03 +0100 Subject: [PATCH 06/11] machine-id-setup: rework --commit based on chase() --- src/shared/machine-id-setup.c | 48 +++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/src/shared/machine-id-setup.c b/src/shared/machine-id-setup.c index 5526a900f4..5fa987a245 100644 --- a/src/shared/machine-id-setup.c +++ b/src/shared/machine-id-setup.c @@ -250,8 +250,6 @@ finish: } int machine_id_commit(const char *root) { - _cleanup_close_ int fd = -EBADF, initial_mntns_fd = -EBADF; - const char *etc_machine_id; sd_id128_t id; int r; @@ -274,9 +272,17 @@ int machine_id_commit(const char *root) { * in a mount namespace, a new file is created at the right place. Afterwards the mount is also removed in the * original mount namespace, thus revealing the file that was just created. */ - etc_machine_id = prefix_roota(root, "/etc/machine-id"); + _cleanup_close_ int etc_fd = -EBADF; + _cleanup_free_ char *etc = NULL; + r = chase("/etc/", root, CHASE_PREFIX_ROOT, &etc, &etc_fd); + if (r < 0) + return log_error_errno(r, "Failed to open /etc/: %m"); - r = path_is_mount_point(etc_machine_id); + _cleanup_free_ char *etc_machine_id = path_join(etc, "machine-id"); + if (!etc_machine_id) + return log_oom(); + + r = fd_is_mount_point(etc_fd, "machine-id", /* flags= */ 0); if (r < 0) return log_error_errno(r, "Failed to determine whether %s is a mount point: %m", etc_machine_id); if (r == 0) { @@ -285,9 +291,12 @@ int machine_id_commit(const char *root) { } /* Read existing machine-id */ - fd = open(etc_machine_id, O_RDONLY|O_CLOEXEC|O_NOCTTY); + + _cleanup_close_ int fd = xopenat_full(etc_fd, "machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW, XO_REGULAR, MODE_INVALID); if (fd < 0) - return log_error_errno(errno, "Cannot open %s: %m", etc_machine_id); + return log_error_errno(fd, "Cannot open %s: %m", etc_machine_id); + + etc_fd = safe_close(etc_fd); r = fd_is_temporary_fs(fd); if (r < 0) @@ -301,10 +310,8 @@ int machine_id_commit(const char *root) { if (r < 0) return log_error_errno(r, "We didn't find a valid machine ID in %s: %m", etc_machine_id); - fd = safe_close(fd); - /* Store current mount namespace */ - initial_mntns_fd = namespace_open_by_type(NAMESPACE_MOUNT); + _cleanup_close_ int initial_mntns_fd = namespace_open_by_type(NAMESPACE_MOUNT); if (initial_mntns_fd < 0) return log_error_errno(initial_mntns_fd, "Can't fetch current mount namespace: %m"); @@ -313,15 +320,23 @@ int machine_id_commit(const char *root) { if (r < 0) return log_error_errno(r, "Failed to set up new mount namespace: %m"); - r = umount_verbose(LOG_ERR, etc_machine_id, 0); + /* Open /etc/ again after we transitioned into our own private mount namespace */ + _cleanup_close_ int etc_fd_again = -EBADF; + r = chase("/etc/", root, CHASE_PREFIX_ROOT, /* ret_path= */ NULL, &etc_fd_again); + if (r < 0) + return log_error_errno(r, "Failed to open /etc/: %m"); + + r = umountat_detach_verbose(LOG_ERR, etc_fd_again, "machine-id"); if (r < 0) return r; /* Update a persistent version of etc_machine_id */ - r = id128_write(etc_machine_id, ID128_FORMAT_PLAIN | ID128_SYNC_ON_WRITE, id); + r = id128_write_at(etc_fd_again, "machine-id", ID128_FORMAT_PLAIN | ID128_SYNC_ON_WRITE, id); if (r < 0) return log_error_errno(r, "Cannot write %s. This is mandatory to get a persistent machine ID: %m", etc_machine_id); + etc_fd_again = safe_close(etc_fd_again); + /* Return to initial namespace and proceed a lazy tmpfs unmount */ r = namespace_enter(/* pidns_fd = */ -EBADF, initial_mntns_fd, @@ -329,10 +344,15 @@ int machine_id_commit(const char *root) { /* userns_fd = */ -EBADF, /* root_fd = */ -EBADF); if (r < 0) - return log_warning_errno(r, "Failed to switch back to initial mount namespace: %m.\nWe'll keep transient %s file until next reboot.", etc_machine_id); + return log_warning_errno(r, + "Failed to switch back to initial mount namespace: %m.\n" + "We'll keep transient %s file until next reboot.", etc_machine_id); - if (umount2(etc_machine_id, MNT_DETACH) < 0) - return log_warning_errno(errno, "Failed to unmount transient %s file: %m.\nWe keep that mount until next reboot.", etc_machine_id); + r = umountat_detach_verbose(LOG_DEBUG, fd, /* filename= */ NULL); + if (r < 0) + return log_warning_errno(r, + "Failed to unmount transient %s file: %m.\n" + "We keep that mount until next reboot.", etc_machine_id); return 0; } From 3773d4dd9ade4ef358f11c8aa9c3df7e1921fa0b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2025 12:19:18 +0100 Subject: [PATCH 07/11] machine-id-setup: remove unnecessary 'else' --- src/shared/machine-id-setup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/machine-id-setup.c b/src/shared/machine-id-setup.c index 5fa987a245..0659bced12 100644 --- a/src/shared/machine-id-setup.c +++ b/src/shared/machine-id-setup.c @@ -204,8 +204,8 @@ int machine_id_setup(const char *root, sd_id128_t machine_id, MachineIdSetupFlag r = id128_write_fd(fd, ID128_FORMAT_PLAIN | ID128_SYNC_ON_WRITE, machine_id); if (r < 0) return log_error_errno(r, "Failed to write %s: %m", etc_machine_id); - else - goto finish; + + goto finish; } } From 64d52739c59e28a5eb453f749e24f23b2d2ec06a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2025 12:09:03 +0100 Subject: [PATCH 08/11] machine-id-setup: rework writing of /etc/machine-id around chase() Let's make sure we properly stay inside of the root fs if one is provided, i.e. stop using prefix_roota() at one more place. --- src/shared/machine-id-setup.c | 139 +++++++++++++++++++++++----------- 1 file changed, 93 insertions(+), 46 deletions(-) diff --git a/src/shared/machine-id-setup.c b/src/shared/machine-id-setup.c index 0659bced12..ee7e08b086 100644 --- a/src/shared/machine-id-setup.c +++ b/src/shared/machine-id-setup.c @@ -131,54 +131,89 @@ static int acquire_machine_id(const char *root, bool machine_id_from_firmware, s } int machine_id_setup(const char *root, sd_id128_t machine_id, MachineIdSetupFlags flags, sd_id128_t *ret) { - const char *etc_machine_id, *run_machine_id; - _cleanup_close_ int fd = -EBADF; + _cleanup_free_ char *etc_machine_id = NULL, *run_machine_id = NULL; bool writable, write_run_machine_id = true; + _cleanup_close_ int fd = -EBADF, run_fd = -EBADF; + bool unlink_run_machine_id = false; int r; - etc_machine_id = prefix_roota(root, "/etc/machine-id"); - WITH_UMASK(0000) { - /* We create this 0444, to indicate that this isn't really - * something you should ever modify. Of course, since the file - * will be owned by root it doesn't matter much, but maybe - * people look. */ + _cleanup_close_ int inode_fd = -EBADF; - (void) mkdir_parents(etc_machine_id, 0755); - fd = open(etc_machine_id, O_RDWR|O_CREAT|O_CLOEXEC|O_NOCTTY, 0444); - if (fd < 0) { - int old_errno = errno; + r = chase("/etc/machine-id", root, CHASE_PREFIX_ROOT, &etc_machine_id, &inode_fd); + if (r == -ENOENT) { + _cleanup_close_ int etc_fd = -EBADF; + _cleanup_free_ char *etc = NULL; - fd = open(etc_machine_id, O_RDONLY|O_CLOEXEC|O_NOCTTY); + r = chase("/etc/", root, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755, &etc, &etc_fd); + if (r < 0) + return log_error_errno(r, "Failed to open '/etc/': %m"); + + etc_machine_id = path_join(etc, "machine-id"); + if (!etc_machine_id) + return log_oom(); + + /* We create this 0444, to indicate that this isn't really something you should ever + * modify. Of course, since the file will be owned by root it doesn't matter much, but maybe + * people look. */ + + fd = openat(etc_fd, "machine-id", O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW|O_CLOEXEC, 0444); if (fd < 0) { - if (old_errno == EROFS && errno == ENOENT) + if (errno == EROFS) return log_error_errno(errno, - "System cannot boot: Missing /etc/machine-id and /etc is mounted read-only.\n" - "Booting up is supported only when:\n" - "1) /etc/machine-id exists and is populated.\n" - "2) /etc/machine-id exists and is empty.\n" - "3) /etc/machine-id is missing and /etc is writable.\n"); - else - return log_error_errno(errno, "Cannot open %s: %m", etc_machine_id); + "System cannot boot: Missing %s and %s/ is read-only.\n" + "Booting up is supported only when:\n" + "1) /etc/machine-id exists and is populated.\n" + "2) /etc/machine-id exists and is empty.\n" + "3) /etc/machine-id is missing and /etc/ is writable.", + etc_machine_id, + etc); + + return log_error_errno(errno, "Cannot create '%s': %m", etc_machine_id); } - writable = false; - } else + log_debug("Successfully opened new '%s' file.", etc_machine_id); writable = true; + } else if (r < 0) + return log_error_errno(r, "Cannot open '/etc/machine-id': %m"); + else { + /* We pinned the inode, now try to convert it into a writable file */ + + fd = xopenat_full(inode_fd, /* path= */ NULL, O_RDWR|O_CLOEXEC, XO_REGULAR, 0444); + if (fd < 0) { + log_debug_errno(fd, "Failed to topen '%s' in writable mode, retrying in read-only mode: %m", etc_machine_id); + + /* If that didn't work, convert it into a readable file */ + fd = xopenat_full(inode_fd, /* path= */ NULL, O_RDONLY|O_CLOEXEC, XO_REGULAR, MODE_INVALID); + if (fd < 0) + return log_error_errno(fd, "Cannot open '%s' in neither writable nor read-only mode: %m", etc_machine_id); + + log_debug("Successfully opened existing '%s' file in read-only mode.", etc_machine_id); + writable = false; + } else { + log_debug("Successfully opened existing '%s' file in writable mode.", etc_machine_id); + writable = true; + } + } } /* A we got a valid machine ID argument, that's what counts */ if (sd_id128_is_null(machine_id) || FLAGS_SET(flags, MACHINE_ID_SETUP_FORCE_FIRMWARE)) { /* Try to read any existing machine ID */ - if (id128_read_fd(fd, ID128_FORMAT_PLAIN, &machine_id) >= 0) + r = id128_read_fd(fd, ID128_FORMAT_PLAIN, &machine_id); + if (r >= 0) goto finish; + log_debug_errno(r, "Unable to read current machine ID, acquiring new one: %m"); + /* Hmm, so, the id currently stored is not useful, then let's acquire one. */ r = acquire_machine_id(root, FLAGS_SET(flags, MACHINE_ID_SETUP_FORCE_FIRMWARE), &machine_id); if (r < 0) return r; - write_run_machine_id = !r; + + write_run_machine_id = !r; /* acquire_machine_id() returns 1 in case we read this machine ID + * from /run/machine-id */ } if (writable) { @@ -209,37 +244,49 @@ int machine_id_setup(const char *root, sd_id128_t machine_id, MachineIdSetupFlag } } - fd = safe_close(fd); - - /* Hmm, we couldn't or shouldn't write the machine-id to /etc? - * So let's write it to /run/machine-id as a replacement */ - - run_machine_id = prefix_roota(root, "/run/machine-id"); + /* Hmm, we couldn't or shouldn't write the machine-id to /etc/? So let's write it to /run/machine-id + * as a replacement */ if (write_run_machine_id) { - WITH_UMASK(0022) - r = id128_write(run_machine_id, ID128_FORMAT_PLAIN, machine_id); - if (r < 0) { - (void) unlink(run_machine_id); - return log_error_errno(r, "Cannot write %s: %m", run_machine_id); + _cleanup_free_ char *run = NULL; + + r = chase("/run/", root, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755, &run, &run_fd); + if (r < 0) + return log_error_errno(r, "Failed to open '/run/': %m"); + + run_machine_id = path_join(run, "machine-id"); + if (!run_machine_id) + return log_oom(); + + WITH_UMASK(0022) { + r = id128_write_at(run_fd, "machine-id", ID128_FORMAT_PLAIN, machine_id); + if (r < 0) + return log_error_errno(r, "Cannot write '%s': %m", run_machine_id); } + + unlink_run_machine_id = true; + } else { + r = chase("/run/machine-id", root, CHASE_PREFIX_ROOT, &run_machine_id, /* ret_inode_fd= */ NULL); + if (r < 0) + return log_error_errno(r, "Failed to open '/run/machine-id': %m"); } /* And now, let's mount it over */ - r = mount_follow_verbose(LOG_ERR, run_machine_id, etc_machine_id, NULL, MS_BIND, NULL); - if (r < 0) { - (void) unlink(run_machine_id); - return r; - } - - log_full(FLAGS_SET(flags, MACHINE_ID_SETUP_FORCE_TRANSIENT) ? LOG_DEBUG : LOG_INFO, "Installed transient %s file.", etc_machine_id); - - /* Mark the mount read-only */ - r = mount_follow_verbose(LOG_WARNING, NULL, etc_machine_id, NULL, MS_BIND|MS_RDONLY|MS_REMOUNT, NULL); + r = mount_follow_verbose(LOG_ERR, run_machine_id, FORMAT_PROC_FD_PATH(fd), /* fstype= */ NULL, MS_BIND, /* options= */ NULL); if (r < 0) return r; + log_full(FLAGS_SET(flags, MACHINE_ID_SETUP_FORCE_TRANSIENT) ? LOG_DEBUG : LOG_INFO, "Installed transient '%s' file.", etc_machine_id); + + unlink_run_machine_id = false; + + /* Mark the mount read-only (note: we are not going via FORMAT_PROC_FD_PATH() here because that fd is not updated to our new bind mount) */ + (void) mount_follow_verbose(LOG_WARNING, /* source= */ NULL, etc_machine_id, /* fstype= */ NULL, MS_BIND|MS_RDONLY|MS_REMOUNT, /* options= */ NULL); + finish: + if (unlink_run_machine_id) + (void) unlinkat(ASSERT_FD(run_fd), "machine-id", /* flags= */ 0); + if (!in_initrd()) (void) sd_notifyf(/* unset_environment= */ false, "X_SYSTEMD_MACHINE_ID=" SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(machine_id)); From 90b9f7a07e6f57825f416f6ce2db0a9f2086754b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2025 13:12:23 +0100 Subject: [PATCH 09/11] chase: introduce flags that verify that chased inode is regular file or dir This also implies the new CHASE_MUST_BE_DIRECTORY flag in case the specified path ends in a slash. This makes the rules stricter, it means we'll be closer to how this is handled in kernel: if a path ends in a slash it can never refer to a non-directory. --- src/basic/chase.c | 20 +++++++++++++++++--- src/basic/chase.h | 2 ++ src/shared/machine-id-setup.c | 14 +++++++------- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index a0edb7e11c..2022e7a169 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -89,6 +89,7 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int int r; assert(!FLAGS_SET(flags, CHASE_PREFIX_ROOT)); + assert(!FLAGS_SET(flags, CHASE_MUST_BE_DIRECTORY|CHASE_MUST_BE_REGULAR)); assert(!FLAGS_SET(flags, CHASE_STEP|CHASE_EXTRACT_FILENAME)); assert(!FLAGS_SET(flags, CHASE_TRAIL_SLASH|CHASE_EXTRACT_FILENAME)); assert(dir_fd >= 0 || dir_fd == AT_FDCWD); @@ -243,8 +244,15 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (root_fd < 0) return -errno; - if (FLAGS_SET(flags, CHASE_TRAIL_SLASH)) - append_trail_slash = ENDSWITH_SET(buffer, "/", "/."); + if (ENDSWITH_SET(buffer, "/", "/.")) { + flags |= CHASE_MUST_BE_DIRECTORY; + if (FLAGS_SET(flags, CHASE_TRAIL_SLASH)) + append_trail_slash = true; + } else if (dot_or_dot_dot(buffer) || endswith(buffer, "/..")) + flags |= CHASE_MUST_BE_DIRECTORY; + + if (FLAGS_SET(flags, CHASE_PARENT)) + flags |= CHASE_MUST_BE_DIRECTORY; for (todo = buffer;;) { _cleanup_free_ char *first = NULL; @@ -476,12 +484,18 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int close_and_replace(fd, child); } - if (FLAGS_SET(flags, CHASE_PARENT)) { + if (FLAGS_SET(flags, CHASE_MUST_BE_DIRECTORY)) { r = stat_verify_directory(&st); if (r < 0) return r; } + if (FLAGS_SET(flags, CHASE_MUST_BE_REGULAR)) { + r = stat_verify_regular(&st); + if (r < 0) + return r; + } + if (ret_path) { if (FLAGS_SET(flags, CHASE_EXTRACT_FILENAME) && done) { _cleanup_free_ char *f = NULL; diff --git a/src/basic/chase.h b/src/basic/chase.h index cedd7097fb..eda7cad0b7 100644 --- a/src/basic/chase.h +++ b/src/basic/chase.h @@ -29,6 +29,8 @@ typedef enum ChaseFlags { * file points to the last path component does not exist. */ CHASE_MKDIR_0755 = 1 << 11, /* Create any missing directories in the given path. */ CHASE_EXTRACT_FILENAME = 1 << 12, /* Only return the last component of the resolved path */ + CHASE_MUST_BE_DIRECTORY = 1 << 13, /* Fail if returned inode fd is not a dir */ + CHASE_MUST_BE_REGULAR = 1 << 14, /* Fail if returned inode fd is not a regular file */ } ChaseFlags; bool unsafe_transition(const struct stat *a, const struct stat *b); diff --git a/src/shared/machine-id-setup.c b/src/shared/machine-id-setup.c index ee7e08b086..bc4d890d35 100644 --- a/src/shared/machine-id-setup.c +++ b/src/shared/machine-id-setup.c @@ -74,7 +74,7 @@ static int acquire_machine_id(const char *root, bool machine_id_from_firmware, s } /* Then, try reading the D-Bus machine ID, unless it is a symlink */ - fd = chase_and_open("/var/lib/dbus/machine-id", root, CHASE_PREFIX_ROOT | CHASE_NOFOLLOW, O_RDONLY|O_CLOEXEC|O_NOCTTY, NULL); + fd = chase_and_open("/var/lib/dbus/machine-id", root, CHASE_PREFIX_ROOT|CHASE_NOFOLLOW|CHASE_MUST_BE_REGULAR, O_RDONLY|O_CLOEXEC|O_NOCTTY, NULL); if (fd >= 0 && id128_read_fd(fd, ID128_FORMAT_PLAIN | ID128_REFUSE_NULL, ret) >= 0) { log_info("Initializing machine ID from D-Bus machine ID."); return 0; @@ -140,12 +140,12 @@ int machine_id_setup(const char *root, sd_id128_t machine_id, MachineIdSetupFlag WITH_UMASK(0000) { _cleanup_close_ int inode_fd = -EBADF; - r = chase("/etc/machine-id", root, CHASE_PREFIX_ROOT, &etc_machine_id, &inode_fd); + r = chase("/etc/machine-id", root, CHASE_PREFIX_ROOT|CHASE_MUST_BE_REGULAR, &etc_machine_id, &inode_fd); if (r == -ENOENT) { _cleanup_close_ int etc_fd = -EBADF; _cleanup_free_ char *etc = NULL; - r = chase("/etc/", root, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755, &etc, &etc_fd); + r = chase("/etc/", root, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755|CHASE_MUST_BE_DIRECTORY, &etc, &etc_fd); if (r < 0) return log_error_errno(r, "Failed to open '/etc/': %m"); @@ -250,7 +250,7 @@ int machine_id_setup(const char *root, sd_id128_t machine_id, MachineIdSetupFlag if (write_run_machine_id) { _cleanup_free_ char *run = NULL; - r = chase("/run/", root, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755, &run, &run_fd); + r = chase("/run/", root, CHASE_PREFIX_ROOT|CHASE_MKDIR_0755|CHASE_MUST_BE_DIRECTORY, &run, &run_fd); if (r < 0) return log_error_errno(r, "Failed to open '/run/': %m"); @@ -266,7 +266,7 @@ int machine_id_setup(const char *root, sd_id128_t machine_id, MachineIdSetupFlag unlink_run_machine_id = true; } else { - r = chase("/run/machine-id", root, CHASE_PREFIX_ROOT, &run_machine_id, /* ret_inode_fd= */ NULL); + r = chase("/run/machine-id", root, CHASE_PREFIX_ROOT|CHASE_MUST_BE_REGULAR, &run_machine_id, /* ret_inode_fd= */ NULL); if (r < 0) return log_error_errno(r, "Failed to open '/run/machine-id': %m"); } @@ -321,7 +321,7 @@ int machine_id_commit(const char *root) { _cleanup_close_ int etc_fd = -EBADF; _cleanup_free_ char *etc = NULL; - r = chase("/etc/", root, CHASE_PREFIX_ROOT, &etc, &etc_fd); + r = chase("/etc/", root, CHASE_PREFIX_ROOT|CHASE_MUST_BE_DIRECTORY, &etc, &etc_fd); if (r < 0) return log_error_errno(r, "Failed to open /etc/: %m"); @@ -369,7 +369,7 @@ int machine_id_commit(const char *root) { /* Open /etc/ again after we transitioned into our own private mount namespace */ _cleanup_close_ int etc_fd_again = -EBADF; - r = chase("/etc/", root, CHASE_PREFIX_ROOT, /* ret_path= */ NULL, &etc_fd_again); + r = chase("/etc/", root, CHASE_PREFIX_ROOT|CHASE_MUST_BE_DIRECTORY, /* ret_path= */ NULL, &etc_fd_again); if (r < 0) return log_error_errno(r, "Failed to open /etc/: %m"); From 3351c0a510483790c0d11b5d9b219a3e0f085583 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 13 Jan 2025 13:20:29 +0100 Subject: [PATCH 10/11] chase: move appending of trailing slash out of loop let's move final processing of the filename out of the loop, and apply it in all cases, uniformly, even if we are asked to only return the final filename. --- src/basic/chase.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/basic/chase.c b/src/basic/chase.c index 2022e7a169..5d064921b5 100644 --- a/src/basic/chase.c +++ b/src/basic/chase.c @@ -91,7 +91,6 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int assert(!FLAGS_SET(flags, CHASE_PREFIX_ROOT)); assert(!FLAGS_SET(flags, CHASE_MUST_BE_DIRECTORY|CHASE_MUST_BE_REGULAR)); assert(!FLAGS_SET(flags, CHASE_STEP|CHASE_EXTRACT_FILENAME)); - assert(!FLAGS_SET(flags, CHASE_TRAIL_SLASH|CHASE_EXTRACT_FILENAME)); assert(dir_fd >= 0 || dir_fd == AT_FDCWD); /* Either the file may be missing, or we return an fd to the final object, but both make no sense */ @@ -263,12 +262,8 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int r = path_find_first_component(&todo, /* accept_dot_dot= */ true, &e); if (r < 0) return r; - if (r == 0) { /* We reached the end. */ - if (append_trail_slash) - if (!strextend(&done, "/")) - return -ENOMEM; + if (r == 0) /* We reached the end. */ break; - } first = strndup(e, r); if (!first) @@ -510,11 +505,15 @@ int chaseat(int dir_fd, const char *path, ChaseFlags flags, char **ret_path, int if (!done) { assert(!need_absolute || FLAGS_SET(flags, CHASE_EXTRACT_FILENAME)); - done = strdup(append_trail_slash ? "./" : "."); + done = strdup("."); if (!done) return -ENOMEM; } + if (append_trail_slash) + if (!strextend(&done, "/")) + return -ENOMEM; + *ret_path = TAKE_PTR(done); } From 09b70f7cc96336290aec66ba31e7e31c7311f6c1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 20 Jan 2025 11:30:32 +0100 Subject: [PATCH 11/11] update TODO --- TODO | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TODO b/TODO index ca887afab3..bc02f7bacf 100644 --- a/TODO +++ b/TODO @@ -91,6 +91,9 @@ Janitorial Clean-ups: * xopenat() should pin the parent dir of the inode it creates before doing its thing, so that it can create, open, label somewhat atomically. +* use CHASE_MUST_BE_DIRECTORY and CHASE_MUST_BE_REGULAR at more places (the + majority of places that currently employ chase() probably should use this) + Deprecations and removals: * Remove any support for booting without /usr pre-mounted in the initrd entirely.