From: Mike Yuan <me@yhndnzj.com>
Date: Tue, 6 Feb 2024 07:33:07 +0000 (+0800)
Subject: process-util: minor follow-up for pidfd_spawn
X-Git-Tag: v256-rc1~964
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c90335403c62075643ccd34042d6d901c8109d33;p=thirdparty%2Fsystemd.git

process-util: minor follow-up for pidfd_spawn
---

diff --git a/src/basic/process-util.c b/src/basic/process-util.c
index ebf3344bf1b..a94ff384091 100644
--- a/src/basic/process-util.c
+++ b/src/basic/process-util.c
@@ -2036,7 +2036,13 @@ int make_reaper_process(bool b) {
 
 DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(posix_spawnattr_t*, posix_spawnattr_destroy, NULL);
 
-int posix_spawn_wrapper(const char *path, char *const *argv, char *const *envp, const char *cgroup, PidRef *ret_pidref) {
+int posix_spawn_wrapper(
+                const char *path,
+                char * const *argv,
+                char * const *envp,
+                const char *cgroup,
+                PidRef *ret_pidref) {
+
         short flags = POSIX_SPAWN_SETSIGMASK|POSIX_SPAWN_SETSIGDEF;
         posix_spawnattr_t attr;
         sigset_t mask;
@@ -2045,7 +2051,13 @@ int posix_spawn_wrapper(const char *path, char *const *argv, char *const *envp,
         /* Forks and invokes 'path' with 'argv' and 'envp' using CLONE_VM and CLONE_VFORK, which means the
          * caller will be blocked until the child either exits or exec's. The memory of the child will be
          * fully shared with the memory of the parent, so that there are no copy-on-write or memory.max
-         * issues. */
+         * issues.
+         *
+         * Also, move the newly-created process into 'cgroup' through POSIX_SPAWN_SETCGROUP (clone3())
+         * if available. Note that CLONE_INTO_CGROUP is only supported on cgroup v2.
+         * returns 1: We're already in the right cgroup
+         *         0: 'cgroup' not specified or POSIX_SPAWN_SETCGROUP is not supported. The caller
+         *            needs to call 'cg_attach' on their own */
 
         assert(path);
         assert(argv);
@@ -2102,10 +2114,7 @@ int posix_spawn_wrapper(const char *path, char *const *argv, char *const *envp,
                 if (r < 0)
                         return r;
 
-                if (cgroup_fd == -EBADF)
-                        return 0; /* We managed to use the new API but we are running under cgroupv1 */
-
-                return 1; /* We managed to use the new API and we are already in the new cgroup */
+                return FLAGS_SET(flags, POSIX_SPAWN_SETCGROUP);
         }
         if (!(ERRNO_IS_NOT_SUPPORTED(r) || ERRNO_IS_PRIVILEGE(r)))
                 return -r;
diff --git a/src/basic/process-util.h b/src/basic/process-util.h
index 06c7f0a7d86..0fc31f7086a 100644
--- a/src/basic/process-util.h
+++ b/src/basic/process-util.h
@@ -238,7 +238,12 @@ int get_process_threads(pid_t pid);
 int is_reaper_process(void);
 int make_reaper_process(bool b);
 
-int posix_spawn_wrapper(const char *path, char *const *argv, char *const *envp, const char *cgroup, PidRef *ret_pidref);
+int posix_spawn_wrapper(
+                const char *path,
+                char * const *argv,
+                char * const *envp,
+                const char *cgroup,
+                PidRef *ret_pidref);
 
 int proc_dir_open(DIR **ret);
 int proc_dir_read(DIR *d, pid_t *ret);
diff --git a/src/core/execute.c b/src/core/execute.c
index e5062e1b466..b91513c5da8 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -364,7 +364,6 @@ int exec_spawn(Unit *unit,
         _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
         _cleanup_fdset_free_ FDSet *fdset = NULL;
         _cleanup_fclose_ FILE *f = NULL;
-        bool move_child = true;
         int r;
 
         assert(unit);
@@ -372,10 +371,10 @@ int exec_spawn(Unit *unit,
         assert(unit->manager->executor_fd >= 0);
         assert(command);
         assert(context);
-        assert(ret);
         assert(params);
         assert(params->fds || (params->n_socket_fds + params->n_storage_fds <= 0));
         assert(!params->files_env); /* We fill this field, ensure it comes NULL-initialized to us */
+        assert(ret);
 
         LOG_CONTEXT_PUSH_UNIT(unit);
 
@@ -456,16 +455,15 @@ int exec_spawn(Unit *unit,
                         &pidref);
         if (r < 0)
                 return log_unit_error_errno(unit, r, "Failed to spawn executor: %m");
-        if (r > 0)
-                move_child = false; /* Already in the right cgroup thanks to CLONE_INTO_CGROUP */
-
-        log_unit_debug(unit, "Forked %s as "PID_FMT, command->path, pidref.pid);
-
         /* We add the new process to the cgroup both in the child (so that we can be sure that no user code is ever
          * executed outside of the cgroup) and in the parent (so that we can be sure that when we kill the cgroup the
          * process will be killed too). */
-        if (move_child && subcgroup_path)
+        if (r == 0 && subcgroup_path)
                 (void) cg_attach(SYSTEMD_CGROUP_CONTROLLER, subcgroup_path, pidref.pid);
+        /* r > 0: Already in the right cgroup thanks to CLONE_INTO_CGROUP */
+
+        log_unit_debug(unit, "Forked %s as " PID_FMT " (%s CLONE_INTO_CGROUP)",
+                       command->path, pidref.pid, r > 0 ? "via" : "without");
 
         exec_status_start(&command->exec_status, pidref.pid);