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

[Peter Zijlstra]

On Mon, Nov 02, 2026 at 09:21:14AM -1000, Tejun Heo wrote:

> I see your point. Moving the autogroup handling into the cpu controller
> keeps it better contained though. sched_cgroup_fork() places the new task by
> its cgroup only, and the cpu controller applies the autogroup once the task
> is on the thread list. This isn't a hill I'm going to die on, but it looks
> like the better direction to me. Can you please take a look at the patch
> below?
> 
> diff --git a/kernel/sched/autogroup.h b/kernel/sched/autogroup.h
> --- a/kernel/sched/autogroup.h
> +++ b/kernel/sched/autogroup.h
> @@ -27,12 +27,12 @@ static inline bool task_group_is_autogroup(struct task_group *tg)
>  	return !!tg->autogroup;
>  }
>  
> +extern unsigned int sysctl_sched_autogroup_enabled;
>  extern bool task_wants_autogroup(struct task_struct *p, struct task_group *tg);
>  
>  static inline struct task_group *
>  autogroup_task_group(struct task_struct *p, struct task_group *tg)
>  {
> -	extern unsigned int sysctl_sched_autogroup_enabled;
>  	int enabled = READ_ONCE(sysctl_sched_autogroup_enabled);
>  
>  	if (enabled && task_wants_autogroup(p, tg))
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4958,7 +4958,6 @@ int sched_cgroup_fork(struct task_struct *p, struct kernel_clone_args *kargs)
>  		struct task_group *tg;
>  		tg = container_of(kargs->cset->subsys[cpu_cgrp_id],
>  				  struct task_group, css);
> -		tg = autogroup_task_group(p, tg);
>  		p->sched_task_group = tg;
>  	}
>  #endif
> @@ -9598,6 +9597,15 @@ static void cpu_cgroup_cancel_attach(struct cgroup_taskset *tset)
>  	scx_cgroup_cancel_attach(tset);
>  }
>  
> +#ifdef CONFIG_SCHED_AUTOGROUP
> +static void cpu_cgroup_fork(struct task_struct *p)
> +{
> +	if (READ_ONCE(p->sched_task_group) == &root_task_group &&
> +	    READ_ONCE(sysctl_sched_autogroup_enabled))
> +		sched_move_task(p, true);
> +}
> +#endif
> +
>  #ifdef CONFIG_UCLAMP_TASK_GROUP
>  static void cpu_util_update_eff(struct cgroup_subsys_state *css)
>  {
> @@ -10562,6 +10570,9 @@ struct cgroup_subsys cpu_cgrp_subsys = {
>  	.can_attach	= cpu_cgroup_can_attach,
>  	.attach		= cpu_cgroup_attach,
>  	.cancel_attach	= cpu_cgroup_cancel_attach,
> +#ifdef CONFIG_SCHED_AUTOGROUP
> +	.fork		= cpu_cgroup_fork,
> +#endif
>  	.legacy_cftypes	= cpu_legacy_files,
>  	.dfl_cftypes	= cpu_files,
>  	.early_init	= true,
> 

Right; so by not doing the autogroup assignment in sched_cgroup_fork()
(which is before publishing the task) but moving it to
cgroup_post_fork() (which is post publishing), we solve the race where
setsid() misses the new task, since the task will update post publish.

Now, using cgroup_post_fork() seems weird (also, I'm not sure if this
calls cgroup_subsys::fork for *all* controllers, or only the enabled
ones).

But we can easily move this into sched_post_fork(); all we really need
to ensure is this happens before scx_post_fork(), right?

Something like so?

---
diff --git a/kernel/sched/autogroup.h b/kernel/sched/autogroup.h
index 06c82b2bdfb5..14cf3fde9767 100644
--- a/kernel/sched/autogroup.h
+++ b/kernel/sched/autogroup.h
@@ -19,6 +19,7 @@ struct autogroup {
 	int			nice;
 };
 
+extern unsigned int sysctl_sched_autogroup_enabled;
 extern void autogroup_init(struct task_struct *init_task);
 extern void autogroup_free(struct task_group *tg);
 
@@ -32,7 +33,6 @@ extern bool task_wants_autogroup(struct task_struct *p, struct task_group *tg);
 static inline struct task_group *
 autogroup_task_group(struct task_struct *p, struct task_group *tg)
 {
-	extern unsigned int sysctl_sched_autogroup_enabled;
 	int enabled = READ_ONCE(sysctl_sched_autogroup_enabled);
 
 	if (enabled && task_wants_autogroup(p, tg))
@@ -41,6 +41,16 @@ autogroup_task_group(struct task_struct *p, struct task_group *tg)
 	return tg;
 }
 
+static inline void autogroup_post_fork(struct task_struct *p)
+{
+	/*
+	 * If setsid() missed this new task; move it to the new autogroup now.
+	 */
+	if (READ_ONCE(sysctl_sched_autogroup_enabled) &&
+	    task_wants_autogroup(p, p->sched_task_group))
+		sched_move_task(p, true);
+}
+
 extern int autogroup_path(struct task_group *tg, char *buf, int buflen);
 
 #else /* !CONFIG_SCHED_AUTOGROUP: */
@@ -58,6 +68,8 @@ autogroup_task_group(struct task_struct *p, struct task_group *tg)
 	return tg;
 }
 
+static inline void autogroup_post_fork(struct task_struct *p) { }
+
 static inline int autogroup_path(struct task_group *tg, char *buf, int buflen)
 {
 	return 0;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 04b62c16fa8c..3e05f7c959e6 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4958,7 +4958,13 @@ int sched_cgroup_fork(struct task_struct *p, struct kernel_clone_args *kargs)
 		struct task_group *tg;
 		tg = container_of(kargs->cset->subsys[cpu_cgrp_id],
 				  struct task_group, css);
+		/*
+		 * No autogroup here because this is unsafe vs setsid()
+		 * which will not see this thread yet. Fixed up in
+		 * sched_post_fork().
+		 *
 		tg = autogroup_task_group(p, tg);
+		 */
 		p->sched_task_group = tg;
 	}
 #endif
@@ -4983,6 +4989,7 @@ static void sched_mm_cid_fork(struct task_struct *t);
 
 void sched_post_fork(struct task_struct *p)
 {
+	autogroup_post_fork(p);
 	sched_mm_cid_fork(p);
 	uclamp_post_fork(p);
 	scx_post_fork(p);