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

[Salvatore Bonaccorso]

Hi Michal,

On Fri, Aug 21, 2026 at 06:59:26PM +0200, Michal Koutn?? wrote:
> 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@xxxxxxxxxx> wrote:
> > With an additional reproducer provided by Noah, I could bisect the
> > change down to 
> 
> Good job.
> 
> > 
> > 	commit 260fbcb92bbeacfcd050410fdc2d24ab15044400
> > 	Author: Tejun Heo <tj@xxxxxxxxxx>
> > 	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@xxxxxxxx>
> > 	    Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > 	    Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> > 
> > 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.

Thanks a lot for looking into this problem. Is there something Noah or
I could test? 

Regards,
Salvatore