Re: [PATCH v3 04/24] sched/deadline: Do not divide by zero active CPUs of a root domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched/deadline: race in sched_server_write_common() with sched_cpu_activate()
--

commit 4111950ab95c5b8efd686d2867c9f080dc134211
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

sched/deadline: Do not divide by zero active CPUs of a root domain

This commit guards __dl_sub() and __dl_add() against dividing by zero when
a root domain has no active CPUs, and updates sched_server_write_common()
to reject debugfs writes when the target CPU is not active.

> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index e6a3b516c703b..a4ae092e0a684 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -410,7 +410,12 @@ static ssize_t sched_server_write_common(struct file *filp, const char __user *u
>  			return  -EINVAL;
>  		}
>  
> -		if (!cpu_online(cpu_of(rq)))
> +		/*
> +		 * Nor while it is inactive, going offline or coming online: the
> +		 * server's bandwidth is not counted in its root domain then,
> +		 * which has no active CPU to spread a change over.
> +		 */
> +		if (!cpu_active(cpu_of(rq)))
>  			return -EBUSY;

[Severity: Medium]
As reported by Sashiko on v2 [1], is checking cpu_active(cpu_of(rq)) under
rq_lock_irqsave without holding cpus_read_lock() sufficient to prevent
running dl_server_apply_params() while the CPU still sits in
def_root_domain with its server bandwidth unaccounted?

In sched_cpu_activate(), set_cpu_active(cpu, true) marks the CPU active
before cpuset_cpu_active() attaches rq to the active root domain and
rebuilds bandwidth accounting via dl_rebuild_rd_accounting():

kernel/sched/core.c:sched_cpu_activate() {
	sched_smt_present_inc(cpu);
	set_cpu_active(cpu, true);

	if (sched_smp_initialized) {
		sched_update_numa(cpu, true);
		sched_domains_numa_masks_set(cpu);
		cpuset_cpu_active();
	}
	...
}

If sched_server_write_common() runs in the window after
set_cpu_active(cpu, true) and before cpuset_cpu_active() attaches rq to
its active root domain, cpu_active(cpu_of(rq)) is already true, but
rq->rd is still def_root_domain (where total_bw was cleared to 0) and
rq->online is still 0.

When dl_server_apply_params() then operates on def_root_domain.dl_bw with
old_bw > 0:

kernel/sched/deadline.c:dl_server_apply_params() {
	...
	if (__dl_overflow(dl_b, cap, old_bw, new_bw))
		return -EBUSY;

	if (init) {
		__add_rq_bw(new_bw, &rq->dl);
		__dl_add(dl_b, new_bw, cpus);
		dl_se->dl_bw_attached = 1;
	} else if (dl_se->dl_bw_attached) {
		__dl_sub(dl_b, dl_se->dl_bw, cpus);
		__dl_add(dl_b, new_bw, cpus);
	...
}

Wouldn't lowering the bandwidth (new_bw < old_bw) underflow the unsigned
calculation dl_b->total_bw - old_bw + new_bw in __dl_overflow() and fail
with -EBUSY, while raising the bandwidth (new_bw >= old_bw) underflows
def_root_domain.dl_bw.total_bw in __dl_sub() and bypasses the active root
domain's admission control before dl_rebuild_rd_accounting()
unconditionally adds new_bw into the active root domain?

Similarly, during sched_cpu_deactivate(), set_cpu_active(cpu, false) runs
without holding rq->__lock, so can sched_server_write_common() pass the
!cpu_active(cpu_of(rq)) check just before set_cpu_active(cpu, false)
clears the CPU from cpu_active_mask?

[1] https://lore.kernel.org/r/sashiko-outbox-198153@kernel.org

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