cgroup: fix spurious SIGKILL of CLONE_INTO_CGROUP children

Since commit b69bb476dee9 ("cgroup: fix race between fork and cgroup.kill"), the fork path snapshots the kill_seq of the child's future cgroup into kargs->kill_seq, and cgroup_post_fork() SIGKILLs the child if that cgroup's kill_seq has changed in the meantime, to catch forks racing with a cgroup.kill sweep.

For CLONE_INTO_CGROUP, however, the snapshot in cgroup_css_set_fork() is taken before the target cgroup has been resolved: kargs->cgrp is always NULL at this point (it is only set at the end of the function). So the "if (kargs->cgrp)" branch is dead code and the snapshot always records the kill_seq of the parent's cgroup. cgroup_post_fork() then compares it with the kill_seq of the target cgroup, so the child gets SIGKILLed whenever the two cgroups have been killed a different number of times.

As a result, once cgroup.kill has been written to a cgroup, every child subsequently cloned into it with clone3(CLONE_INTO_CGROUP) is killed on the spot, for as long as the cgroup exists: kill_seq is not exposed to userspace and never resets.

Re-snapshot kill_seq from the target cgroup once it has been resolved, and drop the dead branch at the early snapshot site.

This does not reopen the race fixed by b69bb476dee9. For CLONE_INTO_CGROUP, everything from the snapshot to the check in cgroup_post_fork() runs with cgroup_mutex held, and kill_seq is only ever incremented under cgroup_mutex.

---
 kernel/cgroup/cgroup.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index c3a12fee7528..2d532bf2c0c7 100644
@@ -6873,10 +6873,7 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs)
 	spin_lock_irq(&css_set_lock);
 	cset = task_css_set(current);
 	get_css_set(cset);
-	if (kargs->cgrp)
-		kargs->kill_seq = kargs->cgrp->kill_seq;
-	else
-		kargs->kill_seq = cset->dfl_cgrp->kill_seq;
+	kargs->kill_seq = cset->dfl_cgrp->kill_seq;
 	spin_unlock_irq(&css_set_lock);
 
 	if (!(kargs->flags & CLONE_INTO_CGROUP)) {

@@ -6940,6 +6937,7 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs)
 
 	put_css_set(cset);
 	kargs->cgrp = dst_cgrp;
+	kargs->kill_seq = dst_cgrp->kill_seq;
 	return ret;
 
 err:

2.55.0.897.gb25b4bd76c-goog

Once cgroup.kill had been written to a cgroup, a stale kill_seq snapshot (taken in cgroup_css_set_fork() before the target cgroup was resolved) caused every child subsequently cloned into that cgroup with clone3(CLONE_INTO_CGROUP) to be SIGKILLed on the spot.

Add a regression test: create a cgroup, kill it while it is empty, then clone a child into it and check that the child runs and exits cleanly. On a kernel without the fix, the test fails:

not ok 4 test_cgkill_clone_into_killed

The test is skipped on kernels without clone3() or without CLONE_INTO_CGROUP.

---
 tools/testing/selftests/cgroup/test_kill.c | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

index f6cd23a8ecc7..5ba8e285b2f3 100644
@@ -7,6 +7,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
 #include "kselftest.h"

@@ -261,6 +262,59 @@ static int test_cgkill_forkbomb(const char *root)
 	return ret;
 }
 
+/*
+ * Test that a cgroup that was killed in the past can still be the target
+ * of clone3(CLONE_INTO_CGROUP): writing cgroup.kill must only kill the
+ * tasks in the cgroup at the time of the write, not tasks cloned into
+ * it afterwards.
+ */
+static int test_cgkill_clone_into_killed(const char *root)
+{
+	pid_t pid;
+	int cgroup_fd = -EBADF;
+	int ret = KSFT_FAIL;
+	char *cgroup = NULL;
+
+	cgroup = cg_name(root, "cg_test_clone_into_killed");
+	if (!cgroup)
+		goto cleanup;
+
+	if (cg_create(cgroup))
+		goto cleanup;
+
+	/* Kill the cgroup while it is still empty. */
+	if (cg_write(cgroup, "cgroup.kill", "1"))
+		goto cleanup;
+
+	cgroup_fd = dirfd_open_opath(cgroup);
+	if (cgroup_fd < 0)
+		goto cleanup;
+
+	pid = clone_into_cgroup(cgroup_fd);
+	if (pid < 0) {
+		if (errno == ENOSYS)
+			ret = KSFT_SKIP;
+		goto cleanup;
+	}
+
+	if (pid == 0)
+		exit(EXIT_SUCCESS);
+
+	/* The child must not be SIGKILLed; it has to exit cleanly. */
+	if (clone_reap(pid, WEXITED) != EXIT_SUCCESS)
+		goto cleanup;
+
+	ret = KSFT_PASS;
+
+cleanup:
+	if (cgroup_fd >= 0)
+		close(cgroup_fd);
+	if (cgroup)
+		cg_destroy(cgroup);
+	free(cgroup);
+	return ret;
+}
+
 #define T(x) { x, #x }
 struct cgkill_test {
 	int (*fn)(const char *root);

@@ -269,6 +323,7 @@ struct cgkill_test {
 	T(test_cgkill_simple),
 	T(test_cgkill_tree),
 	T(test_cgkill_forkbomb),
+	T(test_cgkill_clone_into_killed),
 };
 #undef T
 

2.55.0.897.gb25b4bd76c-goog

Thanks for catching this. Overall looks good. Can you please fix the comment where kill_seq is defined in the header. Currently it says kill_seq is serialized by css_set_lock. After your change, it should for normal fork it is serialized by css_set_lock but for clone3(CLONE_INTO_CGROUP), it is serialized by cgroup_mutex.

Orthogonally, we have plans to remove cgroup_mutex dependency from cgroup.kill, so we will need to reevaluate this at that time.

Hello,

Applied 1-2 to cgroup/for-7.3-fixes with the comment above kill_seq updated as Shakeel suggested:

@@ -527,7 +527,10 @@ struct cgroup {
 
 	int nr_threaded_children;	/* # of live threaded child cgroups */
 
-	/* sequence number for cgroup.kill, serialized by css_set_lock. */
+	/*
+	 * Sequence number for cgroup.kill. Incremented with both cgroup_mutex
+	 * and css_set_lock held. Readers hold either one.
+	 */
 	unsigned int kill_seq;
 
 	struct kernfs_node *kn;		/* cgroup kernfs entry */

Thanks.

tejun

Thanks Tejun, and thanks Shakeel for the review.

- Etienne

We hit this in OpenWrt and reached the same conclusion independently before finding your patch, so here is a second data point from a real workload.

The symptom is that re-creating an OCI container fails. procd's service supervisor writes cgroup.kill to /sys/fs/cgroup/services/<service>/<name> when a jailed instance exits, and leaves the directory in place. The next generation of the same instance is started into that same leaf and execs ujail, which clone3()s the container init with CLONE_INTO_CGROUP into a freshly created cgroup under /sys/fs/cgroup/containers/. The container init is SIGKILLed before it executes a single instruction, and every subsequent attempt fails identically for as long as the services leaf lives.

Instrumenting the child confirmed it never reaches its first statement after clone3(). What isolated it was that rmdir() of the services leaf followed immediately by mkdir() of the same path at the same mode makes the failure disappear, while an unrelated cgroup operation in the same window does not, and while the leaf's attributes are byte for byte identical to a freshly created one. That pointed at per-cgroup state exposed in no file, and the snapshot site then explained it: on 6.18.52 the capture is at cgroup.c:6740 while kargs->cgrp is only assigned at :6803, so the else branch is always taken and cgroup_post_fork() ends up comparing two independent counters.

Backporting this patch to 6.18.52 fixes it. With no userspace change at all, and with the killed cgroup still deliberately left in place, three consecutive create attempts that previously failed now succeed:

before: create rc=251, container spuriously left running (3/3)
after: create rc=0, container correctly left created (3/3)

Your selftest in 2/2 reproduces it on the same machine, and behaves exactly as your commit message says:

6.18.52 without 1/2: ok 1 test_cgkill_simple
ok 2 test_cgkill_tree
ok 3 test_cgkill_forkbomb not ok 4 test_cgkill_clone_into_killed

6.18.52 with 1/2: ok 1 test_cgkill_simple
ok 2 test_cgkill_tree
ok 3 test_cgkill_forkbomb ok 4 test_cgkill_clone_into_killed

Tested on x86_64, kernel 6.18.52, with procd/ujail as the OCI runtime; the two kernels differ only by 1/2.

One request about stable, and apologies if this is simply a matter of timing. The patch is in mainline from v7.3-rc2 and carries Cc: stable, but as of 6.18.53 it is not in linux-6.18.y yet. Given that the failure is silent from userspace, the child dies with no diagnostic and kill_seq is visible nowhere, it would be worth queueing for 6.18.y and the other branches carrying b69bb476dee9 ahead of the usual post-release sweep. We are carrying it as a local backport in the meantime.

Thanks for tracking this down.