Re: [PATCH] sched/autogroup: Fix task_group UAF during thread creation

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Oct 30, 2026 at 12:45:34PM +0900, Daehyeon Ko wrote:
> On Mon, Oct 26, 2026 at 12:57:07PM +0100, Peter Zijlstra wrote:
> > I'm not sure this works; afaict this will result in a NULL deref
> > instead.
> 
> Thanks for taking a look.
> 
> The NULL previous cfs_rq should be handled by the existing set_task_rq()
> contract. __sched_fork() initializes p->se.cfs_rq to NULL, and the normal
> sched_cgroup_fork() path reaches set_task_rq() through __set_task_cpu().
> set_task_rq_fair() returns when prev is NULL, before
> cfs_rq_last_update_time(prev), and set_task_rq() then installs the new
> cfs_rq, parent, and depth.
> 
> > Can't we make ksys_setsid() more complicated instead? Have it create the
> > autogroup in advance, move under the tasklist_lock and destroy on
> > failure or something?
> 
> I don't think moving the autogroup move under tasklist_lock alone closes
> the window, because sched_cgroup_fork() runs before copy_process() takes
> tasklist_lock:
> 
>   clone(CLONE_THREAD)              setsid()
>   -------------------              --------
>   sched_cgroup_fork() -> old tg
>                                     create new autogroup
>                                     lock(tasklist_lock)
>                                     move listed threads; child is absent
>                                     unlock(tasklist_lock)
>                                     drop old autogroup reference
>   lock(tasklist_lock)
>   publish child with old tg
> 
> The ordering trace hit this pre-publication sequence seven times. The
> final revalidation is intended to cover precisely this interval.
> 
> Am I missing another ordering which excludes this interleaving?

No, I think you're right. Also, its a mess :/

We have far too many sched hooks here:


free_signal_struct:
  sched_autogroup_exit();

__put_task_struct:
  sched_core_free();

copy_mm:
  sched_cache_fork();

copy_signal:
  sched_autogroup_fork();



copy_process:
 ret = sched_fork()
   scx_pre_fork()
     LOCK scx_fork_rwsem

 cgroup_can_fork()
   LOCK cgroup_threadgroup_rwsem

 ret = sched_cgroup_fork();
   sched_class::task_fork()
   return scx_fork() // requires scx_fork_rwsem;
                     // also requires p->sched_task_group to be correct;
		     // scx ignores auto-group

 LOCK tasklist_lock

 sched_core_fork() // requires tasklist_lock

 LOCK siglock

+ sched_autogroup_fork_finish();

 UNLOCK siglock
 UNLOCK tasklist_lock

 cgroup_post_fork()
   UNLOCK cgroup_threadgroup_rwsem

 sched_post_fork()
   scx_post_fork()
     UNLOCK scx_fork_rwsem



Now, AUTOGROUP depends on CGROUP, so we can make autogroup_move_group()
also use cgroup_threadgroup_rwsem, such that the autogroup and cgroup
logic stays the same?

I'd hate for this to get more complicated than it already is.

I mean, yes we can move sched_core_fork() into the siglock region, but
duplicating most of sched_cgroup_fork() in there just to fix an
autogroup thingy seems weird and inconsistent.