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

[Kirill Kolyshkin]

On Mon, Oct 19, 2026 at 5:57 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> 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?

Correct; my initial version only had this change, and it fixes
both issues.

>
> 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.

Indeed, I added it for "making sense" (rather than for fixing the
issue) after reading through the initial cgroup.kill patchset reviews.
Eric flagged both places in v2, but only one was fixed in v3.

>
> Acked-by: Oleg Nesterov <oleg@xxxxxxxxxx>
>
>
> Either way, I am not sure about SEND_SIG_NOINFO. Perhaps __cgroup_kill()
> should use SEND_SIG_PRIV? I dunno.
>
> Oleg.
>