Re: refcount_t: addition on 0; use-after-free, regression from 260fbcb92bbe ("cgroup: Move dying_tasks cleanup from cgroup_task_release() to cgroup_task_free()")

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

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

Hi Salvatore,

thanks for the nice report and sorry for not so prompt response.

On Sat, Aug 15, 2026 at 09:41:14PM +0200, Salvatore Bonaccorso <carnil@debian.org> wrote:
> With an additional reproducer provided by Noah, I could bisect the
> change down to 

Good job.

> 
> 	commit 260fbcb92bbeacfcd050410fdc2d24ab15044400
> 	Author: Tejun Heo <tj@kernel.org>
> 	Date:   Tue Oct 28 20:19:16 2025 -1000
> 
> 	    cgroup: Move dying_tasks cleanup from cgroup_task_release() to cgroup_task_free()
> 
> 	    Currently, cgroup_task_exit() adds thread group leaders with live member
> 	    threads to their css_set's dying_tasks list (so cgroup.procs iteration can
> 	    still see the leader), and cgroup_task_release() later removes them with
> 	    list_del_init(&task->cg_list).
> 
> 	    An upcoming patch will defer the dying_tasks list addition, moving it from
> 	    cgroup_task_exit() (called from do_exit()) to a new function called from
> 	    finish_task_switch(). However, release_task() (which calls
> 	    cgroup_task_release()) can run either before or after finish_task_switch(),
> 	    creating a race where cgroup_task_release() might try to remove the task from
> 	    dying_tasks before or while it's being added.
> 
> 	    Move the list_del_init() from cgroup_task_release() to cgroup_task_free() to
> 	    fix this race. cgroup_task_free() runs from __put_task_struct(), which is
> 	    always after both paths, making the cleanup safe.
> 
> 	    Cc: Dan Schatzberg <dschatzberg@meta.com>
> 	    Cc: Peter Zijlstra <peterz@infradead.org>
> 	    Signed-off-by: Tejun Heo <tj@kernel.org>
> 
> But there was the suspect that the matching commit might be
> d245698d727a ("cgroup: Defer task cgroup unlink until after the task
> is done switching out").

I see that after 260fbcb92bbea ("cgroup: Move dying_tasks cleanup from
cgroup_task_release() to cgroup_task_free()") it may be possible that
tasks on the dying_tasks list may drop their ->usage to zero (since the
actual unlinking only happens in __put_task_struct).
Most often those would be skipped due to PF_EXITING except for the case
of thread group leaders (which the reproducer stresses) whose refcount
apparently can drop to zero after task->signal->live > 0 made them
iterable :-/

A band-aid fix could be to use tryget_task_struct() in
css_task_iter_next() (I got that hint from a LLM) and "skip" zeroed
tasks. I see that commit fbe3fb103596b ("sched_ext: Replace
tryget_task_struct() with get_task_struct()"), assumes the iterator
always succeeds in obtaining the task reference (which was the
justification of tryget removal). I expect that sched_ext should still
be fine if dying_tasks with zero references are skipped. (What are they?
Tasks which literally no one should be interested in and they're only
waiting for __put_task_struct_rcu_cb() to be called [*]).

(I'm calling that band-aid because it'd resurrect usage of
tryget_task_struct() and it keeps the dying_tasks list a weird place to
be. If anyone has a better idea?)

The commit d245698d727a ("cgroup: Defer task cgroup unlink until after
the task is done switching out") seems a reasonable separation of the
stages to me.

Regards,
Michal


[*] Except for io_uring_drop_tctx_refs() that calls __put_task_struct()
directly (no RCU) but I'd argue the same, that those should not be
possibly iterated.


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