[PATCH] sched/autogroup: Serialize nice rate-limit updates

[PATCH] sched/autogroup: Serialize nice rate-limit updates

[Hui Su]

proc_sched_autogroup_set_nice() uses a function-static timestamp to
rate-limit the expensive sched_group_set_shares() operation.

The timestamp check and update are not serialized. Two concurrent
unprivileged writers can therefore both observe an expired timestamp,
update it, and proceed to sched_group_set_shares(), defeating the
intended 100 ms rate limit.

Serialize the timestamp check and update with a dedicated spinlock.
Evaluate the capability before entering the lock and keep the heavy
autogroup operation outside the critical section.

Fixes: 5091faa449ee ("sched: Add 'autogroup' scheduling feature: automated per session task groups")
Signed-off-by: Hui Su <sh_def@163.com>
---
Testing:
- x86_64 KVM KCSAN forced-overlap reproducer reported concurrent writes
  to the shared timestamp from proc_sched_autogroup_set_nice() on CPUs
  0 and 1.
- x86_64 KVM rate-limit oracle observed two pre-fix writes within 20 ms
  and no post-fix pair within a 100 ms window.
---
 kernel/sched/autogroup.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/autogroup.c b/kernel/sched/autogroup.c
index e380cf9372bb..3469a52dd255 100644
--- a/kernel/sched/autogroup.c
+++ b/kernel/sched/autogroup.c
@@ -230,11 +230,14 @@ __setup("noautogroup", setup_autogroup);
 
 #ifdef CONFIG_PROC_FS
 
+static DEFINE_SPINLOCK(autogroup_nice_lock);
+
 int proc_sched_autogroup_set_nice(struct task_struct *p, int nice)
 {
 	static unsigned long next = INITIAL_JIFFIES;
 	struct autogroup *ag;
 	unsigned long shares;
+	bool admin;
 	int err, idx;
 
 	if (nice < MIN_NICE || nice > MAX_NICE)
@@ -247,11 +250,17 @@ int proc_sched_autogroup_set_nice(struct task_struct *p, int nice)
 	if (nice < 0 && !can_nice(current, nice))
 		return -EPERM;
 
-	/* This is a heavy operation, taking global locks.. */
-	if (!capable(CAP_SYS_ADMIN) && time_before(jiffies, next))
+	admin = capable(CAP_SYS_ADMIN);
+
+	/* Rate-limit this heavy operation, which takes global locks. */
+	spin_lock(&autogroup_nice_lock);
+	if (!admin && time_before(jiffies, next)) {
+		spin_unlock(&autogroup_nice_lock);
 		return -EAGAIN;
+	}
 
 	next = HZ / 10 + jiffies;
+	spin_unlock(&autogroup_nice_lock);
 	ag = autogroup_task_get(p);
 
 	idx = array_index_nospec(nice + 20, 40);

base-commit: 6f8319e3e9a44dd537d17f41565a8453c560a581
-- 
2.55.0

Re: [PATCH] sched/autogroup: Serialize nice rate-limit updates

[Mike Galbraith]

On Thu, 2026-10-08 at 15:44 +0900, Hui Su wrote:
> 
> - x86_64 KVM rate-limit oracle observed two pre-fix writes within 20 ms
>   and no post-fix pair within a 100 ms window.

The intent was to make it slow enough to prevent users from annoying
admins.  If it doesn't do that well enough, by all means lock it up.

	-Mike

Re: [PATCH] sched/autogroup: Serialize nice rate-limit updates

[K Prateek Nayak]

Hello Hui,

On 10/8/2026 12:14 PM, Hui Su wrote:
> proc_sched_autogroup_set_nice() uses a function-static timestamp to
> rate-limit the expensive sched_group_set_shares() operation.
> 
> The timestamp check and update are not serialized. Two concurrent
> unprivileged writers can therefore both observe an expired timestamp,
> update it, and proceed to sched_group_set_shares(), defeating the
> intended 100 ms rate limit.

Is it really that big of a problem?

> 
> Serialize the timestamp check and update with a dedicated spinlock.
> Evaluate the capability before entering the lock and keep the heavy
> autogroup operation outside the critical section.
> 
> Fixes: 5091faa449ee ("sched: Add 'autogroup' scheduling feature: automated per session task groups")
> Signed-off-by: Hui Su <sh_def@163.com>
> ---
> Testing:
> - x86_64 KVM KCSAN forced-overlap reproducer reported concurrent writes
>   to the shared timestamp from proc_sched_autogroup_set_nice() on CPUs
>   0 and 1.
> - x86_64 KVM rate-limit oracle observed two pre-fix writes within 20 ms
>   and no post-fix pair within a 100 ms window.
> ---
>  kernel/sched/autogroup.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/autogroup.c b/kernel/sched/autogroup.c
> index e380cf9372bb..3469a52dd255 100644
> --- a/kernel/sched/autogroup.c
> +++ b/kernel/sched/autogroup.c
> @@ -230,11 +230,14 @@ __setup("noautogroup", setup_autogroup);
>  
>  #ifdef CONFIG_PROC_FS
>  
> +static DEFINE_SPINLOCK(autogroup_nice_lock);
> +
>  int proc_sched_autogroup_set_nice(struct task_struct *p, int nice)
>  {
>  	static unsigned long next = INITIAL_JIFFIES;
>  	struct autogroup *ag;
>  	unsigned long shares;
> +	bool admin;
>  	int err, idx;
>  
>  	if (nice < MIN_NICE || nice > MAX_NICE)
> @@ -247,11 +250,17 @@ int proc_sched_autogroup_set_nice(struct task_struct *p, int nice)
>  	if (nice < 0 && !can_nice(current, nice))
>  		return -EPERM;
>  
> -	/* This is a heavy operation, taking global locks.. */

That comment suggests we don't want to take any global locks but this
adds one more? Is that a good idea?

Also, any reason this is a spinlock and not a mutex?

> -	if (!capable(CAP_SYS_ADMIN) && time_before(jiffies, next))
> +	admin = capable(CAP_SYS_ADMIN);
> +
> +	/* Rate-limit this heavy operation, which takes global locks. */
> +	spin_lock(&autogroup_nice_lock);
> +	if (!admin && time_before(jiffies, next)) {
> +		spin_unlock(&autogroup_nice_lock);
>  		return -EAGAIN;
> +	}
>  
>  	next = HZ / 10 + jiffies;
> +	spin_unlock(&autogroup_nice_lock);

Spinlock looks too heavy-handed for this. You can perhaps do a simple
try_cmpxchg() and the ones that fails it can simply return -EAGAIN.

Thoughts?

>  	ag = autogroup_task_get(p);
>  
>  	idx = array_index_nospec(nice + 20, 40);
> 
> base-commit: 6f8319e3e9a44dd537d17f41565a8453c560a581

-- 
Thanks and Regards,
Prateek

Re: [PATCH] sched/autogroup: Serialize nice rate-limit updates

[Hui Su]

> Is it really that big of a problem?

I agree that the impact is limited. The issue is that concurrent
unprivileged callers can pass the same expired rate-limit window and
weaken the intended throttling of the expensive task-group update path.

> That comment suggests we don't want to take any global locks but this
> adds one more? Is that a good idea?
>
> Spinlock looks too heavy-handed for this. You can perhaps do a simple
> try_cmpxchg() and the ones that fails it can simply return -EAGAIN.
>
> Thoughts?

I agree that try_cmpxchg() is a better fit than adding another global
lock. One unprivileged caller can claim an expired rate-limit window,
while unprivileged callers that lose the cmpxchg can return -EAGAIN
without serializing on a spinlock or entering the expensive update path.

The v2 implementation needs to preserve the existing CAP_SYS_ADMIN behavior:
privileged callers bypass the rate-limit check but still update next. Thus,
an admin caller that loses the cmpxchg must retry rather than return
-EAGAIN.

I'll rework this along those lines for v2. I'll rerun the KCSAN and
rate-limit tests and add coverage for concurrent CAP_SYS_ADMIN callers
to verify that cmpxchg contention does not introduce an -EAGAIN result
for privileged callers.

Thanks for the review.

Thanks and Regards,
Hui