[PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[Qiurong Fang]

From: fangqiurong <fangqiurong@kylinos.cn>

The sub-scheduler disable path deletes the scheduler's kobjects after
releasing scx_enable_mutex, while the root path deletes them before
releasing it. A concurrent enable on the same cgroup can therefore hit
kobject_add() with the same "sub-%llu" name still in the hierarchy and
fail with -EEXIST, tearing down an otherwise healthy scheduler.

Move the two kobject_del() calls above mutex_unlock() to match the root
path.

Fixes: ebeca1f930ea ("sched_ext: Introduce cgroup sub-sched support")
Signed-off-by: fangqiurong <fangqiurong@kylinos.cn>
---
 kernel/sched/ext/sub.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
index a17d84db93bd..1923e3023bff 100644
--- a/kernel/sched/ext/sub.c
+++ b/kernel/sched/ext/sub.c
@@ -1631,6 +1631,12 @@ void scx_sub_disable(struct scx_sched *sch)
 
 	scx_unlink_sched(sch);
 
+	if (sch->sub_kset)
+		kobject_del(&sch->sub_kset->kobj);
+	/* not added if enable failed before scx_sched_sysfs_add() */
+	if (sch->kobj.state_in_sysfs)
+		kobject_del(&sch->kobj);
+
 	mutex_unlock(&scx_enable_mutex);
 
 	/*
@@ -1661,12 +1667,6 @@ void scx_sub_disable(struct scx_sched *sch)
 	 */
 	WRITE_ONCE(sch->dead, true);
 	synchronize_rcu();
-
-	if (sch->sub_kset)
-		kobject_del(&sch->sub_kset->kobj);
-	/* not added if enable failed before scx_sched_sysfs_add() */
-	if (sch->kobj.state_in_sysfs)
-		kobject_del(&sch->kobj);
 }
 
 /* verify that a scheduler can be attached to @cgrp and return the parent */
-- 
2.43.0

Re: [PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[Tejun Heo]

On Tue, Sep 01, 2026 at 06:22:40PM +0800, Qiurong Fang wrote:
> From: fangqiurong <fangqiurong@kylinos.cn>
> 
> The sub-scheduler disable path deletes the scheduler's kobjects after
> releasing scx_enable_mutex, while the root path deletes them before
> releasing it. A concurrent enable on the same cgroup can therefore hit
> kobject_add() with the same "sub-%llu" name still in the hierarchy and
> fail with -EEXIST, tearing down an otherwise healthy scheduler.
> 
> Move the two kobject_del() calls above mutex_unlock() to match the root
> path.

Why is this a problem? The subsched hasn't been fully unloaded yet so if you
try to attach a new one, it's going to fail.

Thanks.

-- 
tejun

Re: [PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[Qiurong Fang]

Hello Tejun,

On Tue, Sep 01, 2026 at 09:56:51AM -1000, Tejun Heo wrote:
> Why is this a problem? The subsched hasn't been fully unloaded yet so if you
> try to attach a new one, it's going to fail.

When the mutex is released, the cgroup has already been given back to the
parent and the exiting scheduler unlinked - its sysfs name is the only
thing left, and only kobject_add() can see it:

  exiting sched (disable workfn)        new sched (enable workfn)
  -----------------------------        ---------------------------
  mutex_lock(&scx_enable_mutex)
    set_cgroup_sched(cgrp, parent)
    scx_unlink_sched(sch)
  mutex_unlock(&scx_enable_mutex)
                                       mutex_lock(&scx_enable_mutex)
                                       find_parent_sched()         ok
                                       create/validate/link sch    ok
                                       scx_sched_sysfs_add()
                                         kobject_add("sub-%llu")  -EEXIST
                                       err_disable: tear down the
                                       otherwise healthy sched
  ops.sub_detach()/exit()      ~100ms
  synchronize_rcu()
  kobject_del("sub-%llu")      too late

The new scheduler passes every check and dies only at sysfs_add(); the
error surfaces via its own ops.exit() while the attach syscall returns 0,
and there is nothing userspace could wait on. This is the case the root
path's comment covers by deleting before unlocking.

Reproduced with a slow (~100ms) ops.exit() and a second struct_ops
instance attaching to the same cgroup in a loop: 40/40 iterations hit
-EEXIST without the patch, 0/40 with it. Happy to turn the reproducer
into a selftest as a follow-up if wanted.

Andrea is right that the concurrent enable must be a different struct_ops
instance (same instance is rejected by the ops->priv check; sequential
unregister+register is serialized by bpf_scx_unreg() flushing the disable
work). Will state that in v2's changelog.

Thanks.

Re: [PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[Tejun Heo]

Hello,

On Wed, Sep 02, 2026 at 11:22:33AM +0800, Qiurong Fang wrote:
> Hello Tejun,
> 
> On Tue, Sep 01, 2026 at 09:56:51AM -1000, Tejun Heo wrote:
> > Why is this a problem? The subsched hasn't been fully unloaded yet so if you
> > try to attach a new one, it's going to fail.
> 
> When the mutex is released, the cgroup has already been given back to the
> parent and the exiting scheduler unlinked - its sysfs name is the only
> thing left, and only kobject_add() can see it:
> 
>   exiting sched (disable workfn)        new sched (enable workfn)
>   -----------------------------        ---------------------------
>   mutex_lock(&scx_enable_mutex)
>     set_cgroup_sched(cgrp, parent)
>     scx_unlink_sched(sch)
>   mutex_unlock(&scx_enable_mutex)
>                                        mutex_lock(&scx_enable_mutex)
>                                        find_parent_sched()         ok
>                                        create/validate/link sch    ok
>                                        scx_sched_sysfs_add()
>                                          kobject_add("sub-%llu")  -EEXIST
>                                        err_disable: tear down the
>                                        otherwise healthy sched
>   ops.sub_detach()/exit()      ~100ms
>   synchronize_rcu()
>   kobject_del("sub-%llu")      too late
> 
> The new scheduler passes every check and dies only at sysfs_add(); the
> error surfaces via its own ops.exit() while the attach syscall returns 0,
> and there is nothing userspace could wait on. This is the case the root
> path's comment covers by deleting before unlocking.
> 
> Reproduced with a slow (~100ms) ops.exit() and a second struct_ops
> instance attaching to the same cgroup in a loop: 40/40 iterations hit
> -EEXIST without the patch, 0/40 with it. Happy to turn the reproducer
> into a selftest as a follow-up if wanted.

How is that distinguishible from new sched being loaded while the previous
one is still there? There are two ways to observe whether a scheduler is in
place - bpf link destruction, which cna propagate through the sched process
exit, and sysfs kobject visibility and the associated uevent notifications.
ops.sub_detach/exit() being called doesn't mean that a subsched is gone -
these are internal cleanup routines. You don't generate userspace visible
indications from those.

You're trying to load a new scheduler when nobody told anybody that the
previous one is gone, declaring that not succeeding a bug and then fixing
that by introducing an actual bug - now you're telling userspace that the
scheduler is gone via uevent while the scheduler is *still* there. Please
stop.

Thanks.

-- 
tejun

Re: [PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[Andrea Righi]

Hello,

On Tue, Sep 01, 2026 at 06:22:40PM +0800, Qiurong Fang wrote:
> From: fangqiurong <fangqiurong@kylinos.cn>
> 
> The sub-scheduler disable path deletes the scheduler's kobjects after
> releasing scx_enable_mutex, while the root path deletes them before
> releasing it. A concurrent enable on the same cgroup can therefore hit
> kobject_add() with the same "sub-%llu" name still in the hierarchy and
> fail with -EEXIST, tearing down an otherwise healthy scheduler.

This race seems legit to me, but I think the commit message should clarify that
the concurrent enable must use a different struct_ops instance. Re-enabling the
same instance is rejected by the ops->priv check and a normal sequential
unregister/register is serialized.

With that:

Reviewed-by: Andrea Righi <arighi@nvidia.com>

Bonus: would it also be possible to add a sched_ext kselftest for this? The race
should be reproducible deterministically by blocking the parent's
ops.sub_detach() callback after the first child has been unlinked, and then
attaching a second struct_ops instance to the same cgroup. Without this change,
the second scheduler should exit because scx_sched_sysfs_add() returns -EEXIST;
with the change, it should attach successfully.

Thanks,
-Andrea

> 
> Move the two kobject_del() calls above mutex_unlock() to match the root
> path.
> 
> Fixes: ebeca1f930ea ("sched_ext: Introduce cgroup sub-sched support")
> Signed-off-by: fangqiurong <fangqiurong@kylinos.cn>
> ---
>  kernel/sched/ext/sub.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index a17d84db93bd..1923e3023bff 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
> @@ -1631,6 +1631,12 @@ void scx_sub_disable(struct scx_sched *sch)
>  
>  	scx_unlink_sched(sch);
>  
> +	if (sch->sub_kset)
> +		kobject_del(&sch->sub_kset->kobj);
> +	/* not added if enable failed before scx_sched_sysfs_add() */
> +	if (sch->kobj.state_in_sysfs)
> +		kobject_del(&sch->kobj);
> +
>  	mutex_unlock(&scx_enable_mutex);
>  
>  	/*
> @@ -1661,12 +1667,6 @@ void scx_sub_disable(struct scx_sched *sch)
>  	 */
>  	WRITE_ONCE(sch->dead, true);
>  	synchronize_rcu();
> -
> -	if (sch->sub_kset)
> -		kobject_del(&sch->sub_kset->kobj);
> -	/* not added if enable failed before scx_sched_sysfs_add() */
> -	if (sch->kobj.state_in_sysfs)
> -		kobject_del(&sch->kobj);
>  }
>  
>  /* verify that a scheduler can be attached to @cgrp and return the parent */
> -- 
> 2.43.0
> 

Re: [PATCH] sched_ext: Delete sub-scheduler kobjects before releasing scx_enable_mutex

[Qiurong Fang]

Hello Tejun,

Understood - it's the expected behavior. Withdrawing the patch.

Thanks for the explanation.