[RFC PATCH] sched: Fix integer overflow issue of cpu weight delta

[RFC PATCH] sched: Fix integer overflow issue of cpu weight delta

[I Hsin Cheng]

In the original calculation of "cpu_weight_nice_read_s64()", the type of
"delta" is set as "int", while "weight" is of type "unsigned long". the
actual difference between "sched_prio_to_weight[prio]" and "weight"
could go beyond the limit of integer when "weight" is large enough.

For example if "weight" is "2147555403" or larger, the correct "prio"
should be "1", however, in the origin implementation , the result will
be "2" due to integer overflow problem, because "71755 - 2147555403 =
-2147483648", which is exactly the minimum value of integer, the macro
"abs()" won't be able to generate an "int" value with "2147483648".

Obviously, for all the weight larger than "sched_prio_to_weight[0]", the
"prio" result should be "1".

Change the type of "last_delta" and "delta" to "long" can solve the
issue.

I think there's one point to concern, do we have a fixed range of
"tg_weight()" ? if it can go beyond "2147555403", than I think this is
worth to be fixed.

Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
---
 kernel/sched/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 95e40895a519..be52a2fff1e3 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -9917,8 +9917,8 @@ static s64 cpu_weight_nice_read_s64(struct cgroup_subsys_state *css,
 				    struct cftype *cft)
 {
 	unsigned long weight = tg_weight(css_tg(css));
-	int last_delta = INT_MAX;
-	int prio, delta;
+	long last_delta = LONG_MAX, delta;
+	int prio;
 
 	/* find the closest nice value to the current weight */
 	for (prio = 0; prio < ARRAY_SIZE(sched_prio_to_weight); prio++) {
-- 
2.43.0

Re: [RFC PATCH] sched: Fix integer overflow issue of cpu weight delta

[Peter Zijlstra]

On Mon, Dec 02, 2024 at 06:13:44PM +0800, I Hsin Cheng wrote:

> I think there's one point to concern, do we have a fixed range of
> "tg_weight()" ? 

kernel/sched/fair.c:__sched_group_set_shares()

  shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES));

  if (tg->shares == shares)
  	return 0;

  tg->shares = shares;

Re: [RFC PATCH] sched: Fix integer overflow issue of cpu weight delta

[I Hsin Cheng]

On Mon, Dec 02, 2024 at 11:41:03AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 02, 2024 at 06:13:44PM +0800, I Hsin Cheng wrote:
> 
> > I think there's one point to concern, do we have a fixed range of
> > "tg_weight()" ? 
> 
> kernel/sched/fair.c:__sched_group_set_shares()
> 
>   shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES));
> 
>   if (tg->shares == shares)
>   	return 0;
> 
>   tg->shares = shares;


Thanks for clarify this ! So tg->shares should be clamped between (1 <<
1) and (1 << 18), No overflow issue should happen as long as we're sure
the value of "shares" won't exceed the range.

Then why don't we make the type of "tg->shares" to "int" or "unsigned"?
so the size of this member can be shrink. Is there any specific reason
to make "tg->shares" as "unsigned long" ? at the same time I want to ask
does "tg->shares" basically means the same as the task group's weight?

I didn't see any explanation about it in the documentation, or I
might've missed it.

Best regards,
Richard Cheng.