[PATCH] cpufreq: schedutil: Fix rate limit overflow

[PATCH] cpufreq: schedutil: Fix rate limit overflow

[Hui Su]

rate_limit_us is unsigned int. On 32-bit systems, multiplying it by
NSEC_PER_USEC is therefore carried out in 32-bit arithmetic before
the result is assigned to freq_update_delay_ns.

For example, writing 4294968 to rate_limit_us wraps the delay from
4294968000 ns to 704 ns. This makes schedutil update far more often
than configured.

Add sugov_update_rate_limit_us() to widen the value to u64 before
converting units. Use it when updating the tunable through sysfs and
when starting the governor, so both paths set the same valid delay.

Fixes: 9bdcb44e391d ("cpufreq: schedutil: New governor based on scheduler utilization data")
Signed-off-by: Hui Su <sh_def@163.com>

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index dff4ee04694c2b90301ed0fa158a509a3423cee1..8bf7a2c34b9f200d8f5776205ea2a7f495e04e69 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -61,6 +61,12 @@ static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
 
 /************************ Governor internals ***********************/
 
+static void sugov_update_rate_limit_us(struct sugov_policy *sg_policy)
+{
+	sg_policy->freq_update_delay_ns =
+		(u64)sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
+}
+
 static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
 {
 	s64 delta_ns;
@@ -606,7 +612,7 @@ rate_limit_us_store(struct gov_attr_set *attr_set, const char *buf, size_t count
 	tunables->rate_limit_us = rate_limit_us;
 
 	list_for_each_entry(sg_policy, &attr_set->policy_list, tunables_hook)
-		sg_policy->freq_update_delay_ns = rate_limit_us * NSEC_PER_USEC;
+		sugov_update_rate_limit_us(sg_policy);
 
 	return count;
 }
@@ -848,7 +854,7 @@ static int sugov_start(struct cpufreq_policy *policy)
 	void (*uu)(struct update_util_data *data, u64 time, unsigned int flags);
 	unsigned int cpu;
 
-	sg_policy->freq_update_delay_ns	= sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
+	sugov_update_rate_limit_us(sg_policy);
 	sg_policy->last_freq_update_time	= 0;
 	sg_policy->next_freq			= 0;
 	sg_policy->work_in_progress		= false;
-- 
2.43.0

Re: [PATCH] cpufreq: schedutil: Fix rate limit overflow

[Zhongqiu Han]

On 8/5/2026 10:39 PM, Hui Su wrote:
> rate_limit_us is unsigned int. On 32-bit systems, multiplying it by
> NSEC_PER_USEC is therefore carried out in 32-bit arithmetic before

The real issue is that NSEC_PER_USEC is a long, which is only 32 bits on
32-bit systems. By contrast, unsigned int is 32 bits on both 32-bit and
64-bit platforms.

> the result is assigned to freq_update_delay_ns.
> 
> For example, writing 4294968 to rate_limit_us wraps the delay from
> 4294968000 ns to 704 ns. This makes schedutil update far more often
> than configured.
> 
> Add sugov_update_rate_limit_us() to widen the value to u64 before
> converting units. Use it when updating the tunable through sysfs and
> when starting the governor, so both paths set the same valid delay.
> 
> Fixes: 9bdcb44e391d ("cpufreq: schedutil: New governor based on scheduler utilization data")
> Signed-off-by: Hui Su <sh_def@163.com>
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index dff4ee04694c2b90301ed0fa158a509a3423cee1..8bf7a2c34b9f200d8f5776205ea2a7f495e04e69 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -61,6 +61,12 @@ static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
>   
>   /************************ Governor internals ***********************/
>   
> +static void sugov_update_rate_limit_us(struct sugov_policy *sg_policy)
> +{
> +	sg_policy->freq_update_delay_ns =
> +		(u64)sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;

Small nit: better to use (s64) to match the type of
freq_update_delay_ns.

Besides, sashiko report one torn read issue,
https://sashiko.dev/#/patchset/20260805143942.805176-1-sh_def%40163.com

On 32-bit systems, the torn-read mechanism itself is pre-existing and
not introduced by this patch. However, this patch does change
its impact: the main difference is that, prior to this patch, the upper
32 bits were effectively always zero. After this change, a previously
written large value(In practice, configuring rate_limit_us to a
very large value, especially one exceeding 4,294,968 us (about 4.29
seconds), is neither reasonable nor common.) may leave non zero upper 32
bits, allowing sugov_should_update_freq() to theoretically observe a
mixed value (old upper 32 bits + new lower 32 bits) and temporarily
treat freq_update_delay_ns as much larger than intended. This could
cause one frequency update to be skipped, but since
cpufreq_update_util() runs in a hot path, the next invocation would be
expected to observe the correct value. Using atomic64 or a lock on this
hot path would be more costly than it's worth. Based on that, I think we
can keep this torn read as is. Please correct me if anyone disagrees.

With the commit log updated,

Reviewed-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>


> +}
> +
>   static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
>   {
>   	s64 delta_ns;
> @@ -606,7 +612,7 @@ rate_limit_us_store(struct gov_attr_set *attr_set, const char *buf, size_t count
>   	tunables->rate_limit_us = rate_limit_us;
>   
>   	list_for_each_entry(sg_policy, &attr_set->policy_list, tunables_hook)
> -		sg_policy->freq_update_delay_ns = rate_limit_us * NSEC_PER_USEC;
> +		sugov_update_rate_limit_us(sg_policy);
>   
>   	return count;
>   }
> @@ -848,7 +854,7 @@ static int sugov_start(struct cpufreq_policy *policy)
>   	void (*uu)(struct update_util_data *data, u64 time, unsigned int flags);
>   	unsigned int cpu;
>   
> -	sg_policy->freq_update_delay_ns	= sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
> +	sugov_update_rate_limit_us(sg_policy);
>   	sg_policy->last_freq_update_time	= 0;
>   	sg_policy->next_freq			= 0;
>   	sg_policy->work_in_progress		= false;


-- 
Thx and BRs,
Zhongqiu Han