Re: [PATCH 01/12] sched: Reject SCHED_FLAG_KEEP_PARAMS before reserving deadline bandwidth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched: bandwidth leak in __sched_setscheduler() under SCHED_FLAG_KEEP_PARAMS
--

commit 7875cab1acc5b85b363d87da8587a9ad5e4642f8
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

sched: Reject SCHED_FLAG_KEEP_PARAMS before reserving deadline bandwidth

This commit moves the sched_dl_overflow() check and sched_reset_on_fork
update in __sched_setscheduler() after the SCHED_FLAG_KEEP_PARAMS class
change check so rejected requests do not leak deadline bandwidth.

> diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> index 3fa447aabe7fb..f0b7b2d52614f 100644
> --- a/kernel/sched/syscalls.c
> +++ b/kernel/sched/syscalls.c
> @@ -655,17 +655,6 @@ int __sched_setscheduler(struct task_struct *p,
>  		goto recheck;
>  	}
>  
> -	/*
> -	 * If setscheduling to SCHED_DEADLINE (or changing the parameters
> -	 * of a SCHED_DEADLINE task) we need to check if enough bandwidth
> -	 * is available.
> -	 */
> -	if ((dl_policy(policy) || dl_task(p)) && sched_dl_overflow(p, policy, attr)) {
> -		retval = -EBUSY;
> -		goto unlock;
> -	}
> -
> -	p->sched_reset_on_fork = reset_on_fork;
>  	oldprio = p->prio;
>  
>  	newprio = __normal_prio(policy, attr->sched_priority, attr->sched_nice);
> @@ -699,6 +688,19 @@ int __sched_setscheduler(struct task_struct *p,
>  		queue_flags |= DEQUEUE_CLASS;
>  	}
>  
> +	/*
> +	 * If setscheduling to SCHED_DEADLINE (or changing the parameters
> +	 * of a SCHED_DEADLINE task) we need to check if enough bandwidth
> +	 * is available. Last of the checks: nothing gives back the
> +	 * bandwidth it reserves.
> +	 */
> +	if ((dl_policy(policy) || dl_task(p)) && sched_dl_overflow(p, policy, attr)) {
> +		retval = -EBUSY;
> +		goto unlock;
> +	}

[Severity: High]
Can __sched_setscheduler() in kernel/sched/syscalls.c still leak
dl_b->total_bw or corrupt rq->dl bandwidth accounting when
SCHED_FLAG_KEEP_PARAMS is set?

When attr->sched_flags & SCHED_FLAG_KEEP_PARAMS is set, the sched_change
block skips __setscheduler_params(p, attr):

kernel/sched/syscalls.c:__sched_setscheduler() {
	...
	scoped_guard (sched_change, p, queue_flags) {

		if (!(attr->sched_flags & SCHED_FLAG_KEEP_PARAMS)) {
			__setscheduler_params(p, attr);
			p->sched_class = next_class;
			p->prio = newprio;
			__setscheduler_dl_pi(newprio, policy, p, scope);
		}
		__setscheduler_uclamp(p, attr);
	...
}

Because __setscheduler_params() is skipped, neither p->policy nor
p->dl.dl_bw is updated. However, SCHED_FLAG_KEEP_PARAMS is only rejected
when prev_class != next_class, and sched_dl_overflow(p, policy, attr) is
still called even when SCHED_FLAG_KEEP_PARAMS is set.

This allows sched_dl_overflow() to modify deadline bandwidth without
updating p->dl.dl_bw or p->policy in two paths:

1. If a non-deadline task (e.g. p->policy == SCHED_NORMAL, so
!task_has_dl_policy(p) and p->dl.dl_bw == 0) is PI-boosted by a
SCHED_DEADLINE waiter via rt_mutex_setprio(), p->sched_class is
&dl_sched_class while p->policy remains SCHED_NORMAL. Calling
sys_sched_setattr() with sched_policy = SCHED_DEADLINE and
SCHED_FLAG_KEEP_PARAMS populates attr.sched_runtime = p->se.slice via
get_params(). In __sched_setscheduler(), both prev_class and next_class
are &dl_sched_class, so the prev_class != next_class check is bypassed.
sched_dl_overflow() then sees dl_policy(policy) && !task_has_dl_policy(p)
and adds new_bw to dl_b->total_bw via __dl_add(), while the
SCHED_FLAG_KEEP_PARAMS check skips __setscheduler_params(), leaving
p->policy as SCHED_NORMAL and p->dl.dl_bw as 0. When p deboosts, new_bw is
never subtracted from dl_b->total_bw.

2. In SYSCALL_DEFINE3(sched_setattr), get_params(p, &attr, 0) reads p's
deadline parameters without holding p->pi_lock or rq->__lock:

kernel/sched/syscalls.c:SYSCALL_DEFINE3(sched_setattr) {
	...
	if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS)
		get_params(p, &attr, 0);

	return sched_setattr(p, &attr);
}

If a concurrent sched_setattr() updates a SCHED_DEADLINE task's parameters
before __sched_setscheduler() acquires task_rq_lock(p, &rf), attr holds
stale parameters where new_bw != p->dl.dl_bw while prev_class ==
next_class == &dl_sched_class. sched_dl_overflow() then updates
dl_b->total_bw and calls dl_change_utilization(p, new_bw) to update
rq->dl.this_bw and rq->dl.running_bw to the stale new_bw, while
__setscheduler_params() is skipped, leaving p->dl.dl_bw out of sync with
dl_b->total_bw and rq->dl.

Should __sched_setscheduler() also reject SCHED_FLAG_KEEP_PARAMS when
policy != p->policy, and skip sched_dl_overflow() when
SCHED_FLAG_KEEP_PARAMS is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793804635.git.mathieu.desnoyers@efficios.com?part=1