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

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

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