From 2d51ea22aad508c64f1bf2a31b0c03754e8aba2c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Apr 2024 23:20:31 +0800 Subject: [PATCH 1/5] core/unit: use IN_SET at one more place --- src/core/unit.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/unit.h b/src/core/unit.h index 0cfd90b3d4..74520a33cb 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -64,7 +64,7 @@ static inline bool UNIT_IS_INACTIVE_OR_FAILED(UnitActiveState t) { } static inline bool UNIT_IS_LOAD_COMPLETE(UnitLoadState t) { - return t >= 0 && t < _UNIT_LOAD_STATE_MAX && t != UNIT_STUB && t != UNIT_MERGED; + return t >= 0 && t < _UNIT_LOAD_STATE_MAX && !IN_SET(t, UNIT_STUB, UNIT_MERGED); } static inline bool UNIT_IS_LOAD_ERROR(UnitLoadState t) { From da130b9ab86faf732051998cbad0251f041b23a5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 13 Apr 2024 19:56:06 +0800 Subject: [PATCH 2/5] cgroup-setup: modernize cg_migrate --- src/shared/cgroup-setup.c | 59 ++++++++++++--------------------------- 1 file changed, 18 insertions(+), 41 deletions(-) diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index f82af4cf43..6896a03da5 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -598,75 +598,52 @@ int cg_migrate( bool done = false; _cleanup_set_free_ Set *s = NULL; int r, ret = 0; - pid_t my_pid; assert(cfrom); assert(pfrom); assert(cto); assert(pto); - s = set_new(NULL); - if (!s) - return -ENOMEM; - - my_pid = getpid_cached(); - do { _cleanup_fclose_ FILE *f = NULL; - pid_t pid = 0; + pid_t pid; + done = true; r = cg_enumerate_processes(cfrom, pfrom, &f); - if (r < 0) { - if (ret >= 0 && r != -ENOENT) - return r; - - return ret; - } + if (r < 0) + return RET_GATHER(ret, r); while ((r = cg_read_pid(f, &pid)) > 0) { - - /* This might do weird stuff if we aren't a - * single-threaded program. However, we - * luckily know we are not */ - if ((flags & CGROUP_IGNORE_SELF) && pid == my_pid) + /* This might do weird stuff if we aren't a single-threaded program. However, we + * luckily know we are. */ + if (FLAGS_SET(flags, CGROUP_IGNORE_SELF) && pid == getpid_cached()) continue; - if (set_get(s, PID_TO_PTR(pid)) == PID_TO_PTR(pid)) + if (set_contains(s, PID_TO_PTR(pid))) continue; - /* Ignore kernel threads. Since they can only - * exist in the root cgroup, we only check for - * them there. */ - if (cfrom && - empty_or_root(pfrom) && + /* Ignore kernel threads. Since they can only exist in the root cgroup, we only + * check for them there. */ + if (cfrom && empty_or_root(pfrom) && pid_is_kernel_thread(pid) > 0) continue; r = cg_attach(cto, pto, pid); if (r < 0) { - if (ret >= 0 && r != -ESRCH) - ret = r; + if (r != -ESRCH) + RET_GATHER(ret, r); } else if (ret == 0) ret = 1; done = false; - r = set_put(s, PID_TO_PTR(pid)); - if (r < 0) { - if (ret >= 0) - return r; - - return ret; - } - } - - if (r < 0) { - if (ret >= 0) - return r; - - return ret; + r = set_ensure_put(&s, /* hash_ops = */ NULL, PID_TO_PTR(pid)); + if (r < 0) + return RET_GATHER(ret, r); } + if (r < 0) + return RET_GATHER(ret, r); } while (!done); return ret; From 8a70edc1f7a1da98521eac4dddafc06f60750696 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 14 Apr 2024 23:21:13 +0800 Subject: [PATCH 3/5] core/dbus-manager: add missing assertion --- src/core/dbus-manager.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index fa85a5e698..59a7f2bd35 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -772,6 +772,7 @@ static int method_generic_unit_operation( assert(message); assert(m); + assert(handler); /* Read the first argument from the command and pass the operation to the specified per-unit * method. */ From ad68a4e58a5b0794e031c48e91336c1f8a75c9e9 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 13 Apr 2024 20:30:16 +0800 Subject: [PATCH 4/5] core/dbus-manager: rephrase the comment for method_get_unit_processes a bit --- src/core/dbus-manager.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 59a7f2bd35..7077d44723 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -948,9 +948,10 @@ static int method_list_units_by_names(sd_bus_message *message, void *userdata, s } static int method_get_unit_processes(sd_bus_message *message, void *userdata, sd_bus_error *error) { - /* Don't load a unit (since it won't have any processes if it's not loaded), but don't insist on the - * unit being loaded (because even improperly loaded units might still have processes around */ - return method_generic_unit_operation(message, userdata, error, bus_unit_method_get_processes, 0); + /* Don't load a unit actively (since it won't have any processes if it's not loaded), but don't + * insist on the unit being loaded either (because even improperly loaded units might still have + * processes around). */ + return method_generic_unit_operation(message, userdata, error, bus_unit_method_get_processes, /* flags = */ 0); } static int method_attach_processes_to_unit(sd_bus_message *message, void *userdata, sd_bus_error *error) { From 58ff2f1e38dc2acf2e0e5a48d47b8fa381567051 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 13 Apr 2024 22:42:22 +0800 Subject: [PATCH 5/5] core/execute: also check cg_is_threaded for clone3() Prompted by #32259 We already have this check in exec_invoke(), i.e. child. But if CLONE_INTO_CGROUP is used, the failure would occur on parent's side, so do the check there too. --- src/basic/process-util.c | 9 ++++++++- src/core/exec-invoke.c | 5 +++-- src/core/execute.c | 5 +++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 351b5e4095..c9d968dee0 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -25,6 +25,7 @@ #include "alloc-util.h" #include "architecture.h" #include "argv-util.h" +#include "cgroup-util.h" #include "dirent-util.h" #include "env-file.h" #include "env-util.h" @@ -2108,7 +2109,13 @@ int posix_spawn_wrapper( return FLAGS_SET(flags, POSIX_SPAWN_SETCGROUP); } - if (!(ERRNO_IS_NOT_SUPPORTED(r) || ERRNO_IS_PRIVILEGE(r))) + if (ERRNO_IS_NOT_SUPPORTED(r)) { + /* clone3() could also return EOPNOTSUPP if the target cgroup is in threaded mode. */ + if (cgroup && cg_is_threaded(cgroup) > 0) + return -EUCLEAN; + + /* clone3() not available? */ + } else if (!ERRNO_IS_PRIVILEGE(r)) return -r; /* Compiled on a newer host, or seccomp&friends blocking clone3()? Fallback, but need to change the diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index f1e1935e40..90e11d0d1e 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -4264,9 +4264,10 @@ int exec_invoke( r = cg_attach_everywhere(params->cgroup_supported, p, 0, NULL, NULL); if (r == -EUCLEAN) { *exit_status = EXIT_CGROUP; - return log_exec_error_errno(context, params, r, "Failed to attach process to cgroup %s " + return log_exec_error_errno(context, params, r, + "Failed to attach process to cgroup '%s', " "because the cgroup or one of its parents or " - "siblings is in the threaded mode: %m", p); + "siblings is in the threaded mode.", p); } if (r < 0) { *exit_status = EXIT_CGROUP; diff --git a/src/core/execute.c b/src/core/execute.c index 0cfbf79946..5a4acd0775 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -456,6 +456,11 @@ int exec_spawn(Unit *unit, environ, cg_unified() > 0 ? subcgroup_path : NULL, &pidref); + if (r == -EUCLEAN && subcgroup_path) + return log_unit_error_errno(unit, r, + "Failed to spawn process into cgroup '%s', because the cgroup " + "or one of its parents or siblings is in the threaded mode.", + subcgroup_path); if (r < 0) return log_unit_error_errno(unit, r, "Failed to spawn executor: %m"); /* We add the new process to the cgroup both in the child (so that we can be sure that no user code is ever