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 <[email protected]> Either way, I am not sure about SEND_SIG_NOINFO. Perhaps __cgroup_kill() should use SEND_SIG_PRIV? I dunno. Oleg.
