Re: [PATCH v2 2/2] sched_ext/scx_flatcg: make cgv_node_less() wraparound-safe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Atomic read-modify-write race in cgrp_cap_budget duplicates cvtime_delta charges.
- [High] Yielding early penalizes a cgroup's cvtime instead of refunding it, and overrunning causes massive unintended penalties due to unsigned underflow.
--

commit 3be5a2d907efdab246afa9bb4e4577fd489ec842
Author: Tao Cui <cuitao@kylinos.cn>

sched_ext/scx_flatcg: make cgv_node_less() wraparound-safe

The commit addresses a potential wraparound issue in cgv_node_less() by
using a signed difference for cvtime comparison instead of a plain '<'.

Link: https://lore.kernel.org/r/3f1ce004-e259-4e72-a5f7-14a5050053bd@linux.dev

> diff --git a/tools/sched_ext/scx_flatcg.bpf.c b/tools/sched_ext/scx_flatcg.bpf.c
> index fec3595818269..3a25cfdc97092 100644
> --- a/tools/sched_ext/scx_flatcg.bpf.c
> +++ b/tools/sched_ext/scx_flatcg.bpf.c
> @@ -144,7 +144,8 @@ static bool cgv_node_less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>  	cgc_a = container_of(a, struct cgv_node, rb_node);
>  	cgc_b = container_of(b, struct cgv_node, rb_node);
>  
> -	return cgc_a->cvtime < cgc_b->cvtime;
> +	/* wrap-safe: cap_budget keeps nodes within 2^63 of each other */
> +	return (s64)(cgc_a->cvtime - cgc_b->cvtime) < 0;
>  }
>  
>  static struct fcg_cpu_ctx *find_cpu_ctx(void)

While reviewing this patch, I noticed a couple of preexisting issues in
tools/sched_ext/scx_flatcg.bpf.c.

tools/sched_ext/scx_flatcg.bpf.c:cgrp_cap_budget()
> 	/*
> 	 * A node which is on the rbtree can't be pointed to from elsewhere yet
> 	 * and thus can't be updated and repositioned. Instead, we collect the
> 	 * vtime deltas separately and apply it asynchronously here.
> 	 */
> 	delta = __sync_fetch_and_sub(&cgc->cvtime_delta, cgc->cvtime_delta);

[Severity: High]
This is a pre-existing issue, but could this atomic subtraction lead to a race
condition with concurrent updates?

In C, the second argument (cgc->cvtime_delta) is evaluated before the atomic
operation. If another CPU adds to cvtime_delta (e.g. via lockless
__sync_fetch_and_add in fcg_stopping) after the second argument is evaluated
but before the atomic subtraction executes, the operation will subtract the
old value from the new value.

This returns the new value, which gets added to cvtime, but the concurrent
addition remains in cvtime_delta and will be added again in the next call.
Would it be safer to use an atomic exchange here, such as
__sync_lock_test_and_set or bpf_atomic_xchg?


tools/sched_ext/scx_flatcg.bpf.c:fcg_dispatch()
> 		__sync_fetch_and_add(&cgc->cvtime_delta,
> 				     (cpuc->cur_at + cgrp_slice_ns - now) *
> 				     FCG_HWEIGHT_ONE / (cgc->hweight ?: 1));

[Severity: High]
This isn't a bug introduced by this patch, but does this logic penalize
cgroups when they yield early?

If a cgroup yields before its slice is fully consumed, cpuc->cur_at +
cgrp_slice_ns - now is positive. Adding this unused time to cvtime_delta
increases cvtime, pushing the deadline further into the future instead of
refunding it.

Additionally, if the cgroup overruns its slice, could the subtraction
underflow? Since the arithmetic is unsigned, an overrun would result in a
huge number which, after division by cgc->hweight, produces a massive
positive value. This would heavily penalize the cgroup by adding days of
virtual time to its cvtime_delta.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901140343.764080-1-cui.tao@linux.dev?part=2