Re: [PATCH] sched_ext/scx_flatcg: Fix cvtime_delta race and add hweight scaling to bypass charging

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect tracking of cgroup virtual time delta in `fcg_dispatch` leads to double charging on early yield and permanent starvation on slice overrun due to sign inversion and unsigned division of negative values.
--

commit 26a5b29cfee7b890fe54a82847b9652c4700912d
Author: Wanwu Li <liwanwu@kylinos.cn>

sched_ext/scx_flatcg: Fix cvtime_delta race and add hweight scaling to bypass charging

This commit fixes a double-charging race condition in cgrp_cap_budget() by
using __sync_fetch_and_and() to read and clear cvtime_delta atomically. It
also updates the bypass charging path in fcg_stopping() to scale the raw
execution time by the inverse of the cgroup hweight before adding it to
cvtime_delta.

> diff --git a/tools/sched_ext/scx_flatcg.bpf.c b/tools/sched_ext/scx_flatcg.bpf.c
> index fec3595818269..0fd214cc61dae 100644
> --- a/tools/sched_ext/scx_flatcg.bpf.c
> +++ b/tools/sched_ext/scx_flatcg.bpf.c

[ ... ]

> @@ -570,7 +570,8 @@ void BPF_STRUCT_OPS(fcg_stopping, struct task_struct *p, bool runnable)
>  	cgc = find_cgrp_ctx(cgrp);
>  	if (cgc) {
>  		__sync_fetch_and_add(&cgc->cvtime_delta,
> -				     p->se.sum_exec_runtime - taskc->bypassed_at);
> +				     (p->se.sum_exec_runtime - taskc->bypassed_at) *
> +				     FCG_HWEIGHT_ONE / (cgc->hweight ?: 1));

[Severity: High]
This is a pre-existing issue, but seeing the hweight scaling logic added
here brings to mind a potential bug with the same pattern in fcg_dispatch().
Does the tracking of the cgroup virtual time delta in fcg_dispatch() suffer
from double-charging and underflow issues?

In fcg_dispatch(), the unused slice time is calculated as:

(cpuc->cur_at + cgrp_slice_ns - now)

Because the cgroup is charged a full slice upfront in try_pick_next_cgroup(),
should this unused time be refunded by subtracting it from cvtime_delta? The
current code uses __sync_fetch_and_add() which adds the positive unused time
and penalizes the cgroup a second time when it yields early.

Also, if the cgroup overruns its slice (now > cur_at + slice), the
calculation underflows. Because FCG_HWEIGHT_ONE is an unsigned value
(1LLU << 16), does the subsequent multiplication and division by cgc->hweight
destroy the two's complement representation of the negative value?

This unsigned division would shift zeros into the most significant bits,
resulting in a huge positive number being added to cvtime_delta. Could this
push the cgroup's virtual time days into the future and cause permanent
starvation?

>  		taskc->bypassed_at = 0;
>  	}
>  	bpf_cgroup_release(cgrp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608042230.3-1-liwanwu9113@163.com?part=1