Re: [PATCH v2] cgroup: Avoid iteration of dying tasks with zero refcount

["Michal Koutný" <mkoutny@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 4086 bytes --]

On Mon, Sep 07, 2026 at 07:27:32PM +0000, Noah Feldt <noah@feldt.systems> wrote:
> Hi Michal,
> 
> thanks for the v2. I tested it on the same production node and with the
> same reproducer (the 2nd PoC) that triggered the original crash.
> 
> Result: it no longer panics, the use-after-free of the reaped leader is
> gone. But the node now hard-locks up instead. dmesg is attached as
> prod.log (module lists trimmed); the relevant part is:
> 
>   watchdog: CPU24: Watchdog detected hard LOCKUP on cpu 24
>   RIP: 0010:native_queued_spin_lock_slowpath+0x2aa/0x2f0
>   Call Trace:
>    _raw_spin_lock_irqsave+0x3d/0x50
>    cgroup_task_dead+0x29/0x140
>    finish_task_switch.isra.0+0x238/0x2c0
>    __schedule+0x4ec/0xfe0
> 
> i.e. one CPU spins forever holding css_set_lock with IRQs disabled, and
> the other CPUs pile up on that spinlock until the NMI watchdog fires on
> several of them.

I bothed the loop...

> 
> > First, I replaced the if() with a while() loop because when one such
> > dying leader could remain on dying_tasks, there can be more of them
> > (matter of effort) and single css_task_iter_advance() won't be
>  enough
> > (we need to skip all such tasks before get_task_struct()).
> 
> The while() is what locks up:
> 
> > +	while (it->task_pos && it->cur_tasks_head == &it->cur_cset->dying_tasks) {
> > +		task = list_entry(it->task_pos, struct task_struct, cg_list);
> > +		if (!atomic_read(&task->signal->live))
> > +			css_task_iter_advance(it);
> > +	}

...and after I hit Send, I realized it might be unnecessary thanks to
implicit loop via `goto repeat;` in css_task_iter_advance(). But then
there's CSS_TASK_ITER_WITH_DEAD which needs additional care
(fortunately, this flag is not used by those userspace users, so the
if-variant would be sufficient to fix the race in non-sched_ext
scenarios).


> 
> A LLM helped me debug this
> 
> When the leader is still live, atomic_read(&task->signal->live) != 0, so
> the if() body is skipped, css_task_iter_advance() is never called,
> it->task_pos never moves and the loop condition stays true forever. A
> live dying-list leader thus spins the loop under css_set_lock -> the hard
> lockup above.
> 
> The loop has to stop on the first live leader (that is exactly the task we
> want to hand out) and only advance past the dead ones. Turning the skip
> into a break makes it terminate. The variant I tested:
> 
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -5209,6 +5209,7 @@
>   */
>  struct task_struct *css_ta
> sk_iter_next(struct css_task_iter *it)
>  {
> +	struct task_struct *task;
>  	unsigned long irqflags;
> 
>  	if (it->cur_task) {
> @@ -5222,6 +5223,22 @@
>  	if (it->flags & CSS_TASK_ITER_SKIPPED)
>  		css_task_iter_advance(it);
> 
> +	/*
> +	 * @it->task_pos was picked on an earlier call. A dying leader stays on
> +	 * dying_tasks until cgroup_task_free(), past its last usage ref drop,
> +	 * so it may have been reaped since and get_task_struct() on it would
> +	 * resurrect a task about to be freed. That last ref is dropped by an
> +	 * RCU callback queued from release_task(), after signal->live hit zero,
> +	 * so a leader still showing live threads in this irq-disabled section
> +	 * can't lose its ref before the section ends.
> +	 */
> +	while (it->task_pos && it->cur_tasks_head == &it->cur_cset->dying_tasks) {
> +		task = list_entry(it->task_pos, struct task_struct, cg_list);
> +		if (atomic_read(&task->signal->live))
> +			break;
> +		css_task_iter_advance(it);
> +	}
> +
>  	if (it->task_pos) {
>  		it->c
> ur_task = list_entry(it->task_pos, struct task_struct,
>  					  cg_list);
> 
> Same reproducer after this change: no panic and no lockup, the node stays
> up under the load that reproduced it before.
> 
> Tested-by: Noah Elias Feldt <N.Feldt@mittwald.de> # while-variant with the break

Thanks, factoring the live count into the loop is what I should've done
with the loop.
Though, the fix loses a bit of elegance. Hm, thinking...

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]