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

[Oleg Nesterov <oleg@redhat.com>]

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.