From 5eee829043b21b8fdfddfa2119a314d612dc4a3d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Dec 2018 21:49:11 +0100 Subject: [PATCH 1/3] nspawn: move cg_unified_flush() invocation out of parse_argv() It has nothing to do with argument parsing, and hence shouldn't be there. --- src/nspawn/nspawn.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 03538d1c2f..d7bdae64a9 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1332,10 +1332,6 @@ static int parse_argv(int argc, char *argv[]) { arg_caps_retain = (arg_caps_retain | plus | (arg_private_network ? 1ULL << CAP_NET_ADMIN : 0)) & ~minus; - r = cg_unified_flush(); - if (r < 0) - return log_error_errno(r, "Failed to determine whether the unified cgroups hierarchy is used: %m"); - e = getenv("SYSTEMD_NSPAWN_CONTAINER_SERVICE"); if (e) arg_container_service_name = e; @@ -4226,6 +4222,12 @@ int main(int argc, char *argv[]) { if (r < 0) goto finish; + r = cg_unified_flush(); + if (r < 0) { + log_error_errno(r, "Failed to determine whether the unified cgroups hierarchy is used: %m"); + goto finish; + } + r = verify_arguments(); if (r < 0) goto finish; From d5455d2f98d213e89470a4d2ec447300f468914f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Dec 2018 21:54:11 +0100 Subject: [PATCH 2/3] nspawn: split out code parsing env vars into a function of its own This then let's us to ensure it's called after we parsed the cmdline, and after we loaded the settings file, so that it these env var settings override everything loaded from there. --- src/nspawn/nspawn.c | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index d7bdae64a9..a2fc93767f 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -431,6 +431,30 @@ static void parse_mount_settings_env(void) { SET_FLAG(arg_mount_settings, MOUNT_APPLY_APIVFS_NETNS, false); } +static void parse_environment(void) { + const char *e; + int r; + + parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_NS_IPC", CLONE_NEWIPC); + parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_NS_PID", CLONE_NEWPID); + parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_NS_UTS", CLONE_NEWUTS); + parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_SYSTEM", CLONE_NEWIPC|CLONE_NEWPID|CLONE_NEWUTS); + + parse_mount_settings_env(); + + r = getenv_bool("SYSTEMD_NSPAWN_USE_CGNS"); + if (r < 0) + arg_use_cgns = cg_ns_supported(); + else + arg_use_cgns = r; + + e = getenv("SYSTEMD_NSPAWN_CONTAINER_SERVICE"); + if (e) + arg_container_service_name = e; + + detect_unified_cgroup_hierarchy_from_environment(); +} + static int parse_argv(int argc, char *argv[]) { enum { ARG_VERSION = 0x100, @@ -539,7 +563,7 @@ static int parse_argv(int argc, char *argv[]) { }; int c, r; - const char *p, *e; + const char *p; uint64_t plus = 0, minus = 0; bool mask_all_settings = false, mask_no_settings = false; @@ -1243,10 +1267,6 @@ static int parse_argv(int argc, char *argv[]) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--network-namespace-path cannot be combined with other network options."); - parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_NS_IPC", CLONE_NEWIPC); - parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_NS_PID", CLONE_NEWPID); - parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_NS_UTS", CLONE_NEWUTS); - parse_share_ns_env("SYSTEMD_NSPAWN_SHARE_SYSTEM", CLONE_NEWIPC|CLONE_NEWPID|CLONE_NEWUTS); if (arg_userns_mode != USER_NAMESPACE_NO) arg_mount_settings |= MOUNT_USE_USERNS; @@ -1254,8 +1274,6 @@ static int parse_argv(int argc, char *argv[]) { if (arg_private_network) arg_mount_settings |= MOUNT_APPLY_APIVFS_NETNS; - parse_mount_settings_env(); - if (!(arg_clone_ns_flags & CLONE_NEWPID) || !(arg_clone_ns_flags & CLONE_NEWUTS)) { arg_register = false; @@ -1332,16 +1350,6 @@ static int parse_argv(int argc, char *argv[]) { arg_caps_retain = (arg_caps_retain | plus | (arg_private_network ? 1ULL << CAP_NET_ADMIN : 0)) & ~minus; - e = getenv("SYSTEMD_NSPAWN_CONTAINER_SERVICE"); - if (e) - arg_container_service_name = e; - - r = getenv_bool("SYSTEMD_NSPAWN_USE_CGNS"); - if (r < 0) - arg_use_cgns = cg_ns_supported(); - else - arg_use_cgns = r; - r = custom_mount_check_all(); if (r < 0) return r; @@ -4222,6 +4230,8 @@ int main(int argc, char *argv[]) { if (r < 0) goto finish; + parse_environment(); + r = cg_unified_flush(); if (r < 0) { log_error_errno(r, "Failed to determine whether the unified cgroups hierarchy is used: %m"); From 60f1ec13ed059e412c2a2ee4cc3093e2d520673c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Dec 2018 22:00:00 +0100 Subject: [PATCH 3/3] nspawn: move most validation checks and configuration mangling into verify_arguments() That's what the function is for after all, and only if it's done there we can verify the effect of .nspawn files correctly too: after all we should not just validate that everything configured on the command line makes sense, but the stuff configured in the .nspawn files, too. --- src/nspawn/nspawn.c | 144 ++++++++++++++++++++------------------------ 1 file changed, 64 insertions(+), 80 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index a2fc93767f..f4f7c1fd1f 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1257,16 +1257,37 @@ static int parse_argv(int argc, char *argv[]) { assert_not_reached("Unhandled option"); } - /* If --network-namespace-path is given with any other network-related option, - * we need to error out, to avoid conflicts between different network options. */ - if (arg_network_namespace_path && - (arg_network_interfaces || arg_network_macvlan || - arg_network_ipvlan || arg_network_veth_extra || - arg_network_bridge || arg_network_zone || - arg_network_veth || arg_private_network)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--network-namespace-path cannot be combined with other network options."); + if (argc > optind) { + strv_free(arg_parameters); + arg_parameters = strv_copy(argv + optind); + if (!arg_parameters) + return log_oom(); + arg_settings_mask |= SETTING_START_MODE; + } + + if (arg_ephemeral && arg_template && !arg_directory) + /* User asked for ephemeral execution but specified --template= instead of --directory=. Semantically + * such an invocation makes some sense, see https://github.com/systemd/systemd/issues/3667. Let's + * accept this here, and silently make "--ephemeral --template=" equivalent to "--ephemeral + * --directory=". */ + arg_directory = TAKE_PTR(arg_template); + + arg_caps_retain = (arg_caps_retain | plus | (arg_private_network ? 1ULL << CAP_NET_ADMIN : 0)) & ~minus; + + /* Load all settings from .nspawn files */ + if (mask_no_settings) + arg_settings_mask = 0; + + /* Don't load any settings from .nspawn files */ + if (mask_all_settings) + arg_settings_mask = _SETTINGS_MASK_ALL; + + return 1; +} + +static int verify_arguments(void) { + int r; if (arg_userns_mode != USER_NAMESPACE_NO) arg_mount_settings |= MOUNT_USE_USERNS; @@ -1278,111 +1299,74 @@ static int parse_argv(int argc, char *argv[]) { !(arg_clone_ns_flags & CLONE_NEWUTS)) { arg_register = false; if (arg_start_mode != START_PID1) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--boot cannot be used without namespacing."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--boot cannot be used without namespacing."); } if (arg_userns_mode == USER_NAMESPACE_PICK) arg_userns_chown = true; + if (arg_start_mode == START_BOOT && arg_kill_signal <= 0) + arg_kill_signal = SIGRTMIN+3; + if (arg_keep_unit && arg_register && cg_pid_get_owner_uid(0, NULL) >= 0) /* Save the user from accidentally registering either user-$SESSION.scope or user@.service. * The latter is not technically a user session, but we don't need to labour the point. */ - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--keep-unit --register=yes may not be used when invoked from a user session."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--keep-unit --register=yes may not be used when invoked from a user session."); if (arg_directory && arg_image) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--directory= and --image= may not be combined."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--directory= and --image= may not be combined."); if (arg_template && arg_image) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--template= and --image= may not be combined."); - - if (arg_ephemeral && arg_template && !arg_directory) { - /* User asked for ephemeral execution but specified --template= instead of --directory=. Semantically - * such an invocation makes some sense, see https://github.com/systemd/systemd/issues/3667. Let's - * accept this here, and silently make "--ephemeral --template=" equivalent to "--ephemeral - * --directory=". */ - - arg_directory = TAKE_PTR(arg_template); - } + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--template= and --image= may not be combined."); if (arg_template && !(arg_directory || arg_machine)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--template= needs --directory= or --machine=."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--template= needs --directory= or --machine=."); if (arg_ephemeral && arg_template) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--ephemeral and --template= may not be combined."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--ephemeral and --template= may not be combined."); if (arg_ephemeral && !IN_SET(arg_link_journal, LINK_NO, LINK_AUTO)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--ephemeral and --link-journal= may not be combined."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--ephemeral and --link-journal= may not be combined."); if (arg_userns_mode != USER_NAMESPACE_NO && !userns_supported()) - return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), - "--private-users= is not supported, kernel compiled without user namespace support."); + return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "--private-users= is not supported, kernel compiled without user namespace support."); if (arg_userns_chown && arg_read_only) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--read-only and --private-users-chown may not be combined."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--read-only and --private-users-chown may not be combined."); + + /* If --network-namespace-path is given with any other network-related option, + * we need to error out, to avoid conflicts between different network options. */ + if (arg_network_namespace_path && + (arg_network_interfaces || arg_network_macvlan || + arg_network_ipvlan || arg_network_veth_extra || + arg_network_bridge || arg_network_zone || + arg_network_veth || arg_private_network)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--network-namespace-path cannot be combined with other network options."); if (arg_network_bridge && arg_network_zone) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--network-bridge= and --network-zone= may not be combined."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--network-bridge= and --network-zone= may not be combined."); - if (argc > optind) { - arg_parameters = strv_copy(argv + optind); - if (!arg_parameters) - return log_oom(); + if (arg_userns_mode != USER_NAMESPACE_NO && (arg_mount_settings & MOUNT_APPLY_APIVFS_NETNS) && !arg_private_network) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid namespacing settings. Mounting sysfs with --private-users requires --private-network."); - arg_settings_mask |= SETTING_START_MODE; - } + if (arg_userns_mode != USER_NAMESPACE_NO && !(arg_mount_settings & MOUNT_APPLY_APIVFS_RO)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot combine --private-users with read-write mounts."); - /* Load all settings from .nspawn files */ - if (mask_no_settings) - arg_settings_mask = 0; + if (arg_volatile_mode != VOLATILE_NO && arg_read_only) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot combine --read-only with --volatile. Note that --volatile already implies a read-only base hierarchy."); - /* Don't load any settings from .nspawn files */ - if (mask_all_settings) - arg_settings_mask = _SETTINGS_MASK_ALL; + if (arg_expose_ports && !arg_private_network) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot use --port= without private networking."); - arg_caps_retain = (arg_caps_retain | plus | (arg_private_network ? 1ULL << CAP_NET_ADMIN : 0)) & ~minus; +#if ! HAVE_LIBIPTC + if (arg_expose_ports) + return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "--port= is not supported, compiled without libiptc support."); +#endif r = custom_mount_check_all(); if (r < 0) return r; - return 1; -} - -static int verify_arguments(void) { - if (arg_userns_mode != USER_NAMESPACE_NO && (arg_mount_settings & MOUNT_APPLY_APIVFS_NETNS) && !arg_private_network) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid namespacing settings. Mounting sysfs with --private-users requires --private-network."); - - if (arg_userns_mode != USER_NAMESPACE_NO && !(arg_mount_settings & MOUNT_APPLY_APIVFS_RO)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Cannot combine --private-users with read-write mounts."); - - if (arg_volatile_mode != VOLATILE_NO && arg_read_only) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Cannot combine --read-only with --volatile. Note that --volatile already implies a read-only base hierarchy."); - - if (arg_expose_ports && !arg_private_network) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Cannot use --port= without private networking."); - -#if ! HAVE_LIBIPTC - if (arg_expose_ports) - return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), - "--port= is not supported, compiled without libiptc support."); -#endif - - if (arg_start_mode == START_BOOT && arg_kill_signal <= 0) - arg_kill_signal = SIGRTMIN+3; - return 0; }