Re: [PATCH 1/2] sched_ext: Auto-register/unregister dl_server reservations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Modifying a deadline server's parameters via debugfs while its CPU is inactive (offline) corrupts the root domain's bandwidth accounting and can trigger a divide-by-zero kernel panic.
--

commit 81a64c3f8b34ea0965999ebcdc61714354b79af3
Author: Andrea Righi <arighi@nvidia.com>

sched_ext: Auto-register/unregister dl_server reservations

This commit makes deadline server bandwidth reservations dynamic by attaching
or detaching them based on whether a BPF scheduler is loaded. This prevents
inactive scheduling classes from artificially holding capacity.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index ddfd6bc63ab1d..90f6589297ac5 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
[ ... ]
> @@ -1893,7 +1904,8 @@ void __dl_server_attach_root(struct sched_dl_entity *dl_se, struct rq *rq)
>  
>  int dl_server_apply_params(struct sched_dl_entity *dl_se, u64 runtime, u64 period, bool init)
>  {
> -	u64 old_bw = init ? 0 : to_ratio(dl_se->dl_period, dl_se->dl_runtime);
> +	u64 old_bw = (init || !dl_se->dl_bw_attached) ? 0 :
> +		     to_ratio(dl_se->dl_period, dl_se->dl_runtime);
>  	u64 new_bw = to_ratio(period, runtime);
>  	struct rq *rq = dl_se->rq;
>  	int cpu = cpu_of(rq);
> @@ -1913,7 +1925,8 @@ int dl_server_apply_params(struct sched_dl_entity *dl_se, u64 runtime, u64 perio
>  	if (init) {
>  		__add_rq_bw(new_bw, &rq->dl);
>  		__dl_add(dl_b, new_bw, cpus);
> -	} else {
> +	} else if (dl_se->dl_bw_attached) {
>  		__dl_sub(dl_b, dl_se->dl_bw, cpus);

[Severity: High]
This is a pre-existing issue, but can modifying a deadline server's
parameters via debugfs while its CPU is offline trigger a divide-by-zero
panic here?

Looking at dl_server_apply_params(), it retrieves the number of active CPUs
via dl_bw_cpus(cpu) and then passes that to __dl_sub() and __dl_add() when
the server is attached:

dl_server_apply_params() {
    ...
    cpus = dl_bw_cpus(cpu);
    ...
    if (init) {
        ...
    } else if (dl_se->dl_bw_attached) {
        __dl_sub(dl_b, dl_se->dl_bw, cpus);
        __dl_add(dl_b, new_bw, cpus);
        ...
}

If the CPU is offline, dl_bw_cpus() can return 0. The helpers __dl_sub()
and __dl_add() perform a division by cpus:

__dl_sub() {
    ...
    __dl_update(dl_b, (s32)tsk_bw / cpus);
}

Wouldn't this result in a direct divide-by-zero if the debugfs file for an
inactive CPU is written to?

Additionally, even if cpus > 0, since inactive CPUs are skipped during
root domain bandwidth rebuilds, does subtracting dl_se->dl_bw from
dl_b->total_bw without a cpu_active() check subtract a value that was
never added, resulting in accounting corruption?

>  		__dl_add(dl_b, new_bw, cpus);
>  
>  		dl_rq_change_utilization(rq, dl_se, new_bw);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526164420.638711-1-arighi@nvidia.com?part=1