From 45519fd6304aae453d95c6cf11bfc8539c38494c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 29 Sep 2015 22:39:49 +0200 Subject: [PATCH 1/9] systemctl: various modernizations --- src/shared/dropin.c | 5 ++--- src/systemctl/systemctl.c | 47 +++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/shared/dropin.c b/src/shared/dropin.c index 963d05d32e7..1845068adb1 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -78,7 +78,7 @@ int write_drop_in(const char *dir, const char *unit, unsigned level, if (r < 0) return r; - mkdir_p(p, 0755); + (void) mkdir_p(p, 0755); return write_string_file_atomic_label(q, data); } @@ -132,8 +132,7 @@ static int iterate_dir( if (errno == ENOENT) return 0; - log_error_errno(errno, "Failed to open directory %s: %m", path); - return -errno; + return log_error_errno(errno, "Failed to open directory %s: %m", path); } for (;;) { diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 9718ef54df1..621c931c0ba 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -5907,7 +5907,7 @@ static int is_system_running(int argc, char *argv[], void *userdata) { } static int create_edit_temp_file(const char *new_path, const char *original_path, char **ret_tmp_fn) { - char *t; + _cleanup_free_ char *t = NULL; int r; assert(new_path); @@ -5919,26 +5919,21 @@ static int create_edit_temp_file(const char *new_path, const char *original_path return log_error_errno(r, "Failed to determine temporary filename for \"%s\": %m", new_path); r = mkdir_parents(new_path, 0755); - if (r < 0) { - free(t); + if (r < 0) return log_error_errno(r, "Failed to create directories for \"%s\": %m", new_path); - } r = copy_file(original_path, t, 0, 0644, 0); if (r == -ENOENT) { + r = touch(t); - if (r < 0) { - log_error_errno(r, "Failed to create temporary file \"%s\": %m", t); - free(t); - return r; - } - } else if (r < 0) { - log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t); - free(t); - return r; - } + if (r < 0) + return log_error_errno(r, "Failed to create temporary file \"%s\": %m", t); + + } else if (r < 0) + return log_error_errno(r, "Failed to copy \"%s\" to \"%s\": %m", original_path, t); *ret_tmp_fn = t; + t = NULL; return 0; } @@ -5946,6 +5941,9 @@ static int create_edit_temp_file(const char *new_path, const char *original_path static int get_file_to_edit(const char *name, const char *user_home, const char *user_runtime, char **ret_path) { _cleanup_free_ char *path = NULL, *path2 = NULL, *run = NULL; + assert(name); + assert(ret_path); + switch (arg_scope) { case UNIT_FILE_SYSTEM: path = path_join(arg_root, SYSTEM_CONFIG_UNIT_PATH, name); @@ -5997,8 +5995,7 @@ static int get_file_to_edit(const char *name, const char *user_home, const char } static int unit_file_create_dropin(const char *unit_name, const char *user_home, const char *user_runtime, char **ret_new_path, char **ret_tmp_path) { - char *tmp_new_path, *ending; - char *tmp_tmp_path; + char *tmp_new_path, *tmp_tmp_path, *ending; int r; assert(unit_name); @@ -6030,8 +6027,7 @@ static int unit_file_create_copy( char **ret_new_path, char **ret_tmp_path) { - char *tmp_new_path; - char *tmp_tmp_path; + char *tmp_new_path, *tmp_tmp_path; int r; assert(fragment_path); @@ -6150,7 +6146,7 @@ static int run_editor(char **paths) { if (r < 0) return log_error_errno(r, "Failed to wait for child: %m"); - return r; + return 0; } static int find_paths_to_edit(sd_bus *bus, char **names, char ***paths) { @@ -6234,13 +6230,14 @@ static int edit(int argc, char *argv[], void *userdata) { goto end; STRV_FOREACH_PAIR(original, tmp, paths) { - /* If the temporary file is empty we ignore it. - * It's useful if the user wants to cancel its modification + /* If the temporary file is empty we ignore it. It's + * useful if the user wants to cancel its modification */ if (null_or_empty_path(*tmp)) { - log_warning("Editing \"%s\" canceled: temporary file is empty", *original); + log_warning("Editing \"%s\" canceled: temporary file is empty.", *original); continue; } + r = rename(*tmp, *original); if (r < 0) { r = log_error_errno(errno, "Failed to rename \"%s\" to \"%s\": %m", *tmp, *original); @@ -6248,12 +6245,14 @@ static int edit(int argc, char *argv[], void *userdata) { } } - if (!arg_no_reload && bus && !install_client_side()) + r = 0; + + if (!arg_no_reload && !install_client_side()) r = daemon_reload(argc, argv, userdata); end: STRV_FOREACH_PAIR(original, tmp, paths) - unlink_noerrno(*tmp); + (void) unlink(*tmp); return r; } From a4420f7b8ed73b05ef6f31622101e7804daef69f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 29 Sep 2015 22:40:05 +0200 Subject: [PATCH 2/9] systemctl: when reading legacy -t argument for shutdown, don't drop following parameter We currently completely ignore the following parameter, but we really should not, as that is actually the time to shut down on. --- src/systemctl/systemctl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 621c931c0ba..a965369c0c8 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -7003,7 +7003,7 @@ static int shutdown_parse_argv(int argc, char *argv[]) { assert(argc >= 0); assert(argv); - while ((c = getopt_long(argc, argv, "HPrhkKt:afFc", options, NULL)) >= 0) + while ((c = getopt_long(argc, argv, "HPrhkKtafFc", options, NULL)) >= 0) switch (c) { case ARG_HELP: @@ -7486,6 +7486,10 @@ static int logind_schedule_shutdown(void) { case ACTION_KEXEC: action = "kexec"; break; + case ACTION_EXIT: + action = "exit"; + break; + case ACTION_REBOOT: default: action = "reboot"; break; From eb7ec838600b2749208aab2fc9ff9ad3995f89d9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 10:56:07 +0200 Subject: [PATCH 3/9] update TODO --- TODO | 5 ----- 1 file changed, 5 deletions(-) diff --git a/TODO b/TODO index 1f5c31578a0..066d0ae6b62 100644 --- a/TODO +++ b/TODO @@ -26,13 +26,8 @@ External: Features: -* add "requires=" deps on slices from services, not just "wants=" - * add a concept of RemainAfterExit= to scope units -* add sd_booted() check into "systemctl is-system-running", and return - a new state "foreign" or so if we are not running on systemd. - * add journal vacuum by max number of files * add a new command "systemctl revert" or so, that removes all dropin From ee30f6ac32d25b7ab975ee5c288cdefb56a83586 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 12:20:26 +0200 Subject: [PATCH 4/9] nspawn: make sure mount_legacy_cgroup_hierarchy() can deal with NULL root directories --- src/nspawn/nspawn-mount.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index 2bca39f45d2..1b1180ea353 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -534,7 +534,7 @@ static int mount_legacy_cgroup_hierarchy(const char *dest, const char *controlle char *to; int r; - to = strjoina(dest, "/sys/fs/cgroup/", hierarchy); + to = strjoina(strempty(dest), "/sys/fs/cgroup/", hierarchy); r = path_is_mount_point(to, 0); if (r < 0 && r != -ENOENT) From db3b1dedb27b631f6685eda394977249804966c6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 12:24:06 +0200 Subject: [PATCH 5/9] nspawn: order includes --- src/nspawn/nspawn.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 16dfe8a7c16..8bf04d849ba 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -83,12 +83,12 @@ #include "udev-util.h" #include "util.h" -#include "nspawn-settings.h" +#include "nspawn-cgroup.h" +#include "nspawn-expose-ports.h" #include "nspawn-mount.h" #include "nspawn-network.h" -#include "nspawn-expose-ports.h" -#include "nspawn-cgroup.h" #include "nspawn-register.h" +#include "nspawn-settings.h" #include "nspawn-setuid.h" typedef enum ContainerStatus { From 403af78c8049358496ec10920b3aaf741056daf9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 12:48:17 +0200 Subject: [PATCH 6/9] nspawn: fix user namespace support We didn#t actually pass ownership of /run to the UID in the container since some releases, let's fix that. --- src/nspawn/nspawn-mount.c | 7 ++++--- src/nspawn/nspawn-mount.h | 2 +- src/nspawn/nspawn.c | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index 1b1180ea353..85e81b43fe2 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -217,7 +217,8 @@ static int tmpfs_patch_options( } int mount_all(const char *dest, - bool userns, uid_t uid_shift, uid_t uid_range, + bool use_userns, bool in_userns, + uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context) { typedef struct MountPoint { @@ -252,7 +253,7 @@ int mount_all(const char *dest, _cleanup_free_ char *where = NULL, *options = NULL; const char *o; - if (userns != mount_table[k].userns) + if (in_userns != mount_table[k].userns) continue; where = prefix_root(dest, mount_table[k].where); @@ -278,7 +279,7 @@ int mount_all(const char *dest, o = mount_table[k].options; if (streq_ptr(mount_table[k].type, "tmpfs")) { - r = tmpfs_patch_options(o, userns, uid_shift, uid_range, selinux_apifs_context, &options); + r = tmpfs_patch_options(o, use_userns, uid_shift, uid_range, selinux_apifs_context, &options); if (r < 0) return log_oom(); if (r > 0) diff --git a/src/nspawn/nspawn-mount.h b/src/nspawn/nspawn-mount.h index 5abd44cc4ba..da4986add03 100644 --- a/src/nspawn/nspawn-mount.h +++ b/src/nspawn/nspawn-mount.h @@ -57,7 +57,7 @@ int tmpfs_mount_parse(CustomMount **l, unsigned *n, const char *s); int custom_mount_compare(const void *a, const void *b); -int mount_all(const char *dest, bool userns, uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context); +int mount_all(const char *dest, bool use_userns, bool in_userns, uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context); int mount_cgroups(const char *dest, bool unified_requested, bool userns, uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context); int mount_systemd_cgroup_writable(const char *dest, bool unified_requested); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 8bf04d849ba..9f60f41b986 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -2450,7 +2450,7 @@ static int inner_child( } } - r = mount_all(NULL, true, arg_uid_shift, arg_uid_range, arg_selinux_apifs_context); + r = mount_all(NULL, arg_userns, true, arg_uid_shift, arg_uid_range, arg_selinux_apifs_context); if (r < 0) return r; @@ -2701,7 +2701,7 @@ static int outer_child( return log_error_errno(r, "Failed to make tree read-only: %m"); } - r = mount_all(directory, false, arg_uid_shift, arg_uid_range, arg_selinux_apifs_context); + r = mount_all(directory, arg_userns, false, arg_uid_shift, arg_uid_range, arg_selinux_apifs_context); if (r < 0) return r; From d8fc6a000fe21b0c1ba27fbfed8b42d00b349a4b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 13:47:28 +0200 Subject: [PATCH 7/9] nspawn: mount /sys as tmpfs, and then mount only select subdirs of the real sysfs below it This way we can hide things like /sys/firmware or /sys/hypervisor from the container, while keeping the device tree around. While this is a security benefit in itself it also allows us to fix issue #1277. Previously we'd mount /sys before creating the user namespace, in order to be able to mount /sys/fs/cgroup/* beneath it (which resides in it), which we can only mount outside of the user namespace. To ensure that the user namespace owns the network namespace we'd set up the network namespace at the same time as the user namespace. Thus, we'd still see the /sys/class/net/ from the originating network namespace, even though we are in our own network namespace now. With this patch, /sys is mounted before transitioning into the user namespace as tmpfs, so that we can also mount /sys/fs/cgroup/* into it this early. The directories such as /sys/class/ are then later added in from the real sysfs from inside the network and user namespace so that they actually show whatis available in it. Fixes #1277 --- src/nspawn/nspawn-mount.c | 50 ++++++++++++++++++++++++++++++++++++++- src/nspawn/nspawn-mount.h | 1 + src/nspawn/nspawn.c | 4 ++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index 85e81b43fe2..3d302ef9ad0 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -216,6 +216,52 @@ static int tmpfs_patch_options( return !!buf; } +int mount_sysfs(const char *dest) { + const char *full, *top, *x; + + top = prefix_roota(dest, "/sys"); + full = prefix_roota(top, "/full"); + + (void) mkdir(full, 0755); + + if (mount("sysfs", full, "sysfs", MS_RDONLY|MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) < 0) + return log_error_errno(errno, "Failed to mount sysfs to %s: %m", full); + + FOREACH_STRING(x, "block", "bus", "class", "dev", "devices", "kernel") { + _cleanup_free_ char *from = NULL, *to = NULL; + + from = prefix_root(full, x); + if (!from) + return log_oom(); + + to = prefix_root(top, x); + if (!to) + return log_oom(); + + (void) mkdir(to, 0755); + + if (mount(from, to, NULL, MS_BIND, NULL) < 0) + return log_error_errno(errno, "Failed to mount /sys/%s into place: %m", x); + + if (mount(NULL, to, NULL, MS_BIND|MS_RDONLY|MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_REMOUNT, NULL) < 0) + return log_error_errno(errno, "Failed to mount /sys/%s read-only: %m", x); + } + + if (umount(full) < 0) + return log_error_errno(errno, "Failed to unmount %s: %m", full); + + if (rmdir(full) < 0) + return log_error_errno(errno, "Failed to remove %s: %m", full); + + x = prefix_roota(top, "/fs/kdbus"); + (void) mkdir(x, 0755); + + if (mount(NULL, top, NULL, MS_BIND|MS_RDONLY|MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_REMOUNT, NULL) < 0) + return log_error_errno(errno, "Failed to make %s read-only: %m", top); + + return 0; +} + int mount_all(const char *dest, bool use_userns, bool in_userns, uid_t uid_shift, uid_t uid_range, @@ -235,7 +281,7 @@ int mount_all(const char *dest, { "proc", "/proc", "proc", NULL, MS_NOSUID|MS_NOEXEC|MS_NODEV, true, true }, { "/proc/sys", "/proc/sys", NULL, NULL, MS_BIND, true, true }, /* Bind mount first */ { NULL, "/proc/sys", NULL, NULL, MS_BIND|MS_RDONLY|MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_REMOUNT, true, true }, /* Then, make it r/o */ - { "sysfs", "/sys", "sysfs", NULL, MS_RDONLY|MS_NOSUID|MS_NOEXEC|MS_NODEV, true, false }, + { "tmpfs", "/sys", "tmpfs", "mode=755", MS_NOSUID|MS_NOEXEC|MS_NODEV, true, false }, { "tmpfs", "/dev", "tmpfs", "mode=755", MS_NOSUID|MS_STRICTATIME, true, false }, { "tmpfs", "/dev/shm", "tmpfs", "mode=1777", MS_NOSUID|MS_NODEV|MS_STRICTATIME, true, false }, { "tmpfs", "/run", "tmpfs", "mode=755", MS_NOSUID|MS_NODEV|MS_STRICTATIME, true, false }, @@ -570,6 +616,8 @@ static int mount_legacy_cgroups( cgroup_root = prefix_roota(dest, "/sys/fs/cgroup"); + (void) mkdir_p(cgroup_root, 0755); + /* Mount a tmpfs to /sys/fs/cgroup if it's not mounted there yet. */ r = path_is_mount_point(cgroup_root, AT_SYMLINK_FOLLOW); if (r < 0) diff --git a/src/nspawn/nspawn-mount.h b/src/nspawn/nspawn-mount.h index da4986add03..54cab87665d 100644 --- a/src/nspawn/nspawn-mount.h +++ b/src/nspawn/nspawn-mount.h @@ -58,6 +58,7 @@ int tmpfs_mount_parse(CustomMount **l, unsigned *n, const char *s); int custom_mount_compare(const void *a, const void *b); int mount_all(const char *dest, bool use_userns, bool in_userns, uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context); +int mount_sysfs(const char *dest); int mount_cgroups(const char *dest, bool unified_requested, bool userns, uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context); int mount_systemd_cgroup_writable(const char *dest, bool unified_requested); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 9f60f41b986..f4a2e3d9bab 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -2454,6 +2454,10 @@ static int inner_child( if (r < 0) return r; + r = mount_sysfs(NULL); + if (r < 0) + return r; + /* Wait until we are cgroup-ified, so that we * can mount the right cgroup path writable */ if (!barrier_place_and_sync(barrier)) { /* #3 */ From 737af7347c03a6ed2dfeea76647e9c1ca5b05a64 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 14:16:40 +0200 Subject: [PATCH 8/9] log: properly return -EINVAL from log_set_max_level_from_string() If we just return the value we got from log_level_from_string() on failure we'll return -1, which is not a proper error code. log_set_target_from_string() did get this right already, hence let's fix this here too. --- src/basic/log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/log.c b/src/basic/log.c index 38f42b3a6e3..e6d7d151820 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -922,7 +922,7 @@ int log_set_max_level_from_string(const char *e) { t = log_level_from_string(e); if (t < 0) - return t; + return -EINVAL; log_set_max_level(t); return 0; From 2ca2a91cf1deba83825692f1ce06116d2aed2379 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Sep 2015 15:01:01 +0200 Subject: [PATCH 9/9] analyze: add new set-log-target subcommand We already have the property writable, hence let's add a command to set it. --- man/systemd-analyze.xml | 7 +++++++ src/analyze/analyze.c | 42 +++++++++++++++++++++++++++++++++-------- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/man/systemd-analyze.xml b/man/systemd-analyze.xml index 198315052f2..58e6fd17803 100644 --- a/man/systemd-analyze.xml +++ b/man/systemd-analyze.xml @@ -168,6 +168,13 @@ described in systemd1). + systemd-analyze set-log-target + TARGET changes the current log + target of the systemd daemon to + TARGET (accepts the same values as + described in + systemd1). + systemd-analyze verify will load unit files and print warnings if any errors are detected. Files specified on the command line will be loaded, but also any other diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 7054deeae51..f05f1e5581d 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -1217,10 +1217,8 @@ static int dump(sd_bus *bus, char **args) { &error, &reply, ""); - if (r < 0) { - log_error("Failed issue method call: %s", bus_error_message(&error, -r)); - return r; - } + if (r < 0) + return log_error_errno(r, "Failed issue method call: %s", bus_error_message(&error, r)); r = sd_bus_message_read(reply, "s", &text); if (r < 0) @@ -1251,11 +1249,36 @@ static int set_log_level(sd_bus *bus, char **args) { &error, "s", args[0]); - if (r < 0) { - log_error("Failed to issue method call: %s", bus_error_message(&error, -r)); - return -EIO; + if (r < 0) + return log_error_errno(r, "Failed to issue method call: %s", bus_error_message(&error, r)); + + return 0; +} + +static int set_log_target(sd_bus *bus, char **args) { + _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; + int r; + + assert(bus); + assert(args); + + if (strv_length(args) != 1) { + log_error("This command expects one argument only."); + return -E2BIG; } + r = sd_bus_set_property( + bus, + "org.freedesktop.systemd1", + "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", + "LogTarget", + &error, + "s", + args[0]); + if (r < 0) + return log_error_errno(r, "Failed to issue method call: %s", bus_error_message(&error, r)); + return 0; } @@ -1285,7 +1308,8 @@ static void help(void) { " critical-chain Print a tree of the time critical chain of units\n" " plot Output SVG graphic showing service initialization\n" " dot Output dependency graph in dot(1) format\n" - " set-log-level LEVEL Set logging threshold for systemd\n" + " set-log-level LEVEL Set logging threshold for manager\n" + " set-log-target TARGET Set logging target for manager\n" " dump Output state serialization of service manager\n" " verify FILE... Check unit files for correctness\n" , program_invocation_short_name); @@ -1452,6 +1476,8 @@ int main(int argc, char *argv[]) { r = dump(bus, argv+optind+1); else if (streq(argv[optind], "set-log-level")) r = set_log_level(bus, argv+optind+1); + else if (streq(argv[optind], "set-log-target")) + r = set_log_target(bus, argv+optind+1); else log_error("Unknown operation '%s'.", argv[optind]); }