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

[K Prateek Nayak <kprateek.nayak@amd.com>]

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