[PATCH 0/2] cgroup: fix cgroup.kill for exited thread group leader

[PATCH 0/2] cgroup: fix cgroup.kill for exited thread group leader

[Kir Kolyshkin]

cgroup.kill fails to kill a process whose thread group leader has
exited while other threads are still running, leaving the cgroup
populated. This can be used by a process in a container to survive
the container being killed, including the case of a container with a
private PID namespace, where such a process is the container's init.

Patch 1 fixes __cgroup_kill() to walk all threads and to send a
thread-group-directed SIGKILL. Patch 2 adds selftests for both
scenarios described in patch 1; they fail without the fix.

Tested on v7.3-rc7 (x86_64 VM): test_kill passes (30 runs), other
cgroup selftests show no changes compared to the unpatched kernel.

The issue was found in crun, see patch 1 for details.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
Kir Kolyshkin (2):
      cgroup: fix cgroup.kill for exited thread group leader
      selftests/cgroup: test cgroup.kill with an exited thread group leader

 kernel/cgroup/cgroup.c                     |   4 +-
 tools/testing/selftests/cgroup/test_kill.c | 158 +++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+), 2 deletions(-)
---
base-commit: f909e534f5e34feb2ea2701d54df63e21aa506fc
change-id: 20261018-cgroup-kill-exited-leader-a15159e8c230

Best regards,
-- 
Kir Kolyshkin <kolyshkin@gmail.com>

[PATCH 1/2] cgroup: fix cgroup.kill for exited thread group leader

[Kir Kolyshkin]

cgroup.kill does not work in case a process' thread group leader is gone
but other threads are still running. This is because __cgroup_kill()
only walks the thread group leaders and sends SIGKILL to each of them
with send_sig(), which uses PIDTYPE_PID:

1. The exited leader is still listed by CSS_TASK_ITER_PROCS as long as
   the process has live threads, but a signal directed at a single
   exiting task never takes effect: complete_signal() does not look for
   another thread to deliver it to, and __send_signal_locked() now drops
   it altogether.  The process keeps running.

2. The live threads of such a process can be migrated to another cgroup
   by writing any of their IDs to cgroup.procs, while the exited leader
   stays behind.  The destination cgroup has no thread group leader to
   iterate, so writing to its cgroup.kill does nothing at all.

In both cases the cgroup stays populated after cgroup.kill, and a
process inside a container can use this to survive the container being
killed.  This also works with a private PID namespace when the init of
the container is the process, as the PID namespace is not torn down
while init has live threads.

Case 1 can be fixed by using do_send_sig_info(..., PIDTYPE_TGID) instead
of send_sig(), so that the signal is taken by a live thread even if the
task found by the iterator is exiting. Note it was pointed out by Eric
in [1] while reviewing v2 of cgroup.kill patchset, and v3 was meant to
switch to it, but the change was only made in cgroup_post_fork(),
leaving __cgroup_kill() with send_sig().

Case 2 can only be fixed by iterating over all threads, so this is what
the fix also does. Once a thread group is being killed, its other
threads are skipped since they already have a fatal signal pending.

I ran some performance checks and the added cost is ~60 ns per every
thread on my laptop (and less than a millisecond for 10K threads),
which seems low. The time to "populated 0" seems unchanged.

The issue was found in crun by Anthropic's OSS Scanner, and crun is
getting a workaround for it [2] (which does what we needed to do
before cgroup.kill was added).

[1] https://lore.kernel.org/all/m1v97x6niq.fsf@fess.ebiederm.org/
[2] https://github.com/containers/crun/pull/2351

Fixes: 661ee6280931 ("cgroup: introduce cgroup.kill")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 kernel/cgroup/cgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 227d09704ca5..efb7923f0c92 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4289,7 +4289,7 @@ static void __cgroup_kill(struct cgroup *cgrp)
 	cgrp->kill_seq++;
 	spin_unlock_irq(&css_set_lock);
 
-	css_task_iter_start(&cgrp->self, CSS_TASK_ITER_PROCS | CSS_TASK_ITER_THREADED, &it);
+	css_task_iter_start(&cgrp->self, CSS_TASK_ITER_THREADED, &it);
 	while ((task = css_task_iter_next(&it))) {
 		/* Ignore kernel threads here. */
 		if (task->flags & PF_KTHREAD)
@@ -4299,7 +4299,7 @@ static void __cgroup_kill(struct cgroup *cgrp)
 		if (__fatal_signal_pending(task))
 			continue;
 
-		send_sig(SIGKILL, task, 0);
+		do_send_sig_info(SIGKILL, SEND_SIG_NOINFO, task, PIDTYPE_TGID);
 	}
 	css_task_iter_end(&it);
 }

-- 
2.55.0

[PATCH 2/2] selftests/cgroup: test cgroup.kill with an exited thread group leader

[Kir Kolyshkin]

Add two tests for cgroup.kill and a process whose thread group leader
has exited while another thread is still running:

 - test_cgkill_exited_leader kills the cgroup the process is in;
 - test_cgkill_exited_leader_migrated moves the remaining thread to
   another cgroup, where the process is not listed in cgroup.procs, and
   kills that cgroup.

Both check that the cgroup is no longer populated afterwards.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 tools/testing/selftests/cgroup/test_kill.c | 158 +++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)

diff --git a/tools/testing/selftests/cgroup/test_kill.c b/tools/testing/selftests/cgroup/test_kill.c
index bac1ddd8cb94..756bc7193d4b 100644
--- a/tools/testing/selftests/cgroup/test_kill.c
+++ b/tools/testing/selftests/cgroup/test_kill.c
@@ -2,10 +2,13 @@
 
 #include <errno.h>
 #include <linux/limits.h>
+#include <sched.h>
+#include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/syscall.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
@@ -315,6 +318,159 @@ static int test_cgkill_clone_into_killed(const char *root)
 	return ret;
 }
 
+static int sleep_forever(void *arg)
+{
+	for (;;)
+		pause();
+	return 0;
+}
+
+/*
+ * Start a thread and exit the main thread only, leaving behind a zombie
+ * thread group leader with a live thread.
+ */
+static int exit_leader_fn(const char *cgroup, void *arg)
+{
+	const size_t stack_size = 64 * 1024;
+	char *stack = malloc(stack_size);
+
+	if (!stack)
+		return EXIT_FAILURE;
+
+	if (clone(sleep_forever, stack + stack_size,
+		  CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+		  CLONE_THREAD | CLONE_SYSVSEM, NULL) < 0)
+		return EXIT_FAILURE;
+
+	syscall(SYS_exit, 0);
+	return EXIT_FAILURE;
+}
+
+static int wait_for_zombie_leader(pid_t pid)
+{
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		if (!proc_read_strstr(pid, false, "stat", ") Z "))
+			return 0;
+		usleep(10000);
+	}
+
+	return -1;
+}
+
+/*
+ * Test that cgroup.kill kills a process whose thread group leader has
+ * already exited while its other threads are still running.
+ */
+static int test_cgkill_exited_leader(const char *root)
+{
+	int ret = KSFT_FAIL;
+	char *cgroup = NULL;
+	pid_t pid = -ESRCH;
+
+	cgroup = cg_name(root, "cg_test_exited_leader");
+	if (!cgroup)
+		goto cleanup;
+
+	if (cg_create(cgroup))
+		goto cleanup;
+
+	pid = cg_run_nowait(cgroup, exit_leader_fn, NULL);
+	if (pid < 0)
+		goto cleanup;
+
+	if (wait_for_zombie_leader(pid))
+		goto cleanup;
+
+	if (cg_read_strcmp(cgroup, "cgroup.events", "populated 1\n"))
+		goto cleanup;
+
+	if (cg_write(cgroup, "cgroup.kill", "1"))
+		goto cleanup;
+
+	if (cg_read_strcmp_wait(cgroup, "cgroup.events", "populated 0\n"))
+		goto cleanup;
+
+	ret = KSFT_PASS;
+
+cleanup:
+	if (pid > 0) {
+		kill(pid, SIGKILL);
+		wait_for_pid(pid);
+	}
+	if (cgroup)
+		cg_destroy(cgroup);
+	free(cgroup);
+	return ret;
+}
+
+/*
+ * Test that cgroup.kill kills the threads of a process that were migrated
+ * to the cgroup after their thread group leader had exited, and so are
+ * not listed in its cgroup.procs.
+ */
+static int test_cgkill_exited_leader_migrated(const char *root)
+{
+	char *parent = NULL, *src = NULL, *dst = NULL;
+	int ret = KSFT_FAIL;
+	pid_t pid = -ESRCH;
+
+	parent = cg_name(root, "cg_test_exited_leader_migrated");
+	if (!parent)
+		goto cleanup;
+
+	src = cg_name(parent, "src");
+	if (!src)
+		goto cleanup;
+
+	dst = cg_name(parent, "dst");
+	if (!dst)
+		goto cleanup;
+
+	if (cg_create(parent) || cg_create(src) || cg_create(dst))
+		goto cleanup;
+
+	pid = cg_run_nowait(src, exit_leader_fn, NULL);
+	if (pid < 0)
+		goto cleanup;
+
+	if (wait_for_zombie_leader(pid))
+		goto cleanup;
+
+	/* The exiting leader stays behind, only the live thread is moved. */
+	if (cg_enter(dst, pid))
+		goto cleanup;
+
+	if (cg_read_strcmp(dst, "cgroup.events", "populated 1\n") ||
+	    cg_read_strcmp(dst, "cgroup.procs", ""))
+		goto cleanup;
+
+	if (cg_write(dst, "cgroup.kill", "1"))
+		goto cleanup;
+
+	if (cg_read_strcmp_wait(dst, "cgroup.events", "populated 0\n"))
+		goto cleanup;
+
+	ret = KSFT_PASS;
+
+cleanup:
+	if (pid > 0) {
+		kill(pid, SIGKILL);
+		wait_for_pid(pid);
+	}
+	if (dst)
+		cg_destroy(dst);
+	if (src)
+		cg_destroy(src);
+	if (parent)
+		cg_destroy(parent);
+	free(dst);
+	free(src);
+	free(parent);
+	return ret;
+}
+
 #define T(x) { x, #x }
 struct cgkill_test {
 	int (*fn)(const char *root);
@@ -324,6 +480,8 @@ struct cgkill_test {
 	T(test_cgkill_tree),
 	T(test_cgkill_forkbomb),
 	T(test_cgkill_clone_into_killed),
+	T(test_cgkill_exited_leader),
+	T(test_cgkill_exited_leader_migrated),
 };
 #undef T
 

-- 
2.55.0

Re: [PATCH 1/2] cgroup: fix cgroup.kill for exited thread group leader

[Oleg Nesterov]

On 10/18, Kir Kolyshkin wrote:
>
> cgroup.kill does not work in case a process' thread group leader is gone
> but other threads are still running. This is because __cgroup_kill()
> only walks the thread group leaders and sends SIGKILL to each of them
> with send_sig(), which uses PIDTYPE_PID:
>
> 1. The exited leader is still listed by CSS_TASK_ITER_PROCS as long as
>    the process has live threads, but a signal directed at a single
>    exiting task never takes effect: complete_signal() does not look for
>    another thread to deliver it to, and __send_signal_locked() now drops
>    it altogether.  The process keeps running.
>
> 2. The live threads of such a process can be migrated to another cgroup
>    by writing any of their IDs to cgroup.procs, while the exited leader
>    stays behind.  The destination cgroup has no thread group leader to
>    iterate, so writing to its cgroup.kill does nothing at all.

...

> Case 1 can be fixed by using do_send_sig_info(..., PIDTYPE_TGID) instead
> of send_sig(), so that the signal is taken by a live thread even if the
> task found by the iterator is exiting. Note it was pointed out by Eric
> in [1] while reviewing v2 of cgroup.kill patchset, and v3 was meant to
> switch to it, but the change was only made in cgroup_post_fork(),
> leaving __cgroup_kill() with send_sig().
>
> Case 2 can only be fixed by iterating over all threads, so this is what
> the fix also does. Once a thread group is being killed, its other
> threads are skipped since they already have a fatal signal pending.

...

> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -4289,7 +4289,7 @@ static void __cgroup_kill(struct cgroup *cgrp)
>  	cgrp->kill_seq++;
>  	spin_unlock_irq(&css_set_lock);
>
> -	css_task_iter_start(&cgrp->self, CSS_TASK_ITER_PROCS | CSS_TASK_ITER_THREADED, &it);
> +	css_task_iter_start(&cgrp->self, CSS_TASK_ITER_THREADED, &it);

I guess this change alone is enough to fix both problems?

But even if I am right,

> @@ -4299,7 +4299,7 @@ static void __cgroup_kill(struct cgroup *cgrp)
>  		if (__fatal_signal_pending(task))
>  			continue;
>
> -		send_sig(SIGKILL, task, 0);
> +		do_send_sig_info(SIGKILL, SEND_SIG_NOINFO, task, PIDTYPE_TGID);

I agree that do_send_sig_info(PIDTYPE_TGID) makes more sense.

Acked-by: Oleg Nesterov <oleg@redhat.com>


Either way, I am not sure about SEND_SIG_NOINFO. Perhaps __cgroup_kill()
should use SEND_SIG_PRIV? I dunno.

Oleg.