Re: [PATCH] cpufreq: schedutil: Publish util hooks only after all sg_cpu are initialized

[Christian Loehle <christian.loehle@arm.com>]

On 7/16/26 12:51, Zhongqiu Han wrote:
> Commit 16a03c71bba0 ("cpufreq: schedutil: Merge initialization code of
> sg_cpu in single loop") merged the per-CPU initialization and the
> utilization-hook registration into a single loop in sugov_start().
> 
> For a shared cpufreq policy this re-introduces the race originally fixed
> by commit ab2f7cf141aa ("cpufreq: schedutil: Fix sugov_start() versus
> sugov_update_shared() race").
> 
> The scheduler's util path reaches the hook under RCU-sched and never takes
> policy->rwsem, so the rwsem held across sugov_start() cannot serialize the
> two. Once the first CPU's hook is published, sugov_update_shared() may run
> and, via sugov_next_freq_shared(), read/write each sibling sugov_cpu
> (iowait_boost, util, bw_min, ...) concurrently with the memset() still
> initializing them, with no lock common to both sides: the update side holds
> sg_policy->update_lock while the init side holds only policy->rwsem, which
> the scheduler's util path never takes.
> 
> The walk only accesses scalar members, never a pointer like ->sg_policy,
> so it does not crash today; it merely uses stale (or zero on first start)
> values that skew the frequency selection and tracepoints. It is still a
> genuine data race, and a latent crash once any pointer member is
> dereferenced there.
> 
> Restore the two-phase approach: initialize all per-CPU structures first,
> and only then publish the per-CPU utilization update hooks.
> 
> Fixes: 16a03c71bba0 ("cpufreq: schedutil: Merge initialization code of sg_cpu in single loop")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index dc0835ceee98..6ad080de8644 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -872,8 +872,19 @@ static int sugov_start(struct cpufreq_policy *policy)
>  		memset(sg_cpu, 0, sizeof(*sg_cpu));
>  		sg_cpu->cpu = cpu;
>  		sg_cpu->sg_policy = sg_policy;
> +	}
> +
> +	/*
> +	 * Publish the hooks only after all per-CPU data is initialized, so a
> +	 * shared policy's sugov_update_shared() never reads an uninitialized
> +	 * sibling sugov_cpu.
> +	 */
> +	for_each_cpu(cpu, policy->cpus) {
> +		struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
> +
>  		cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util, uu);
>  	}
> +
>  	return 0;
>  }
>  


Good catch!
Reviewed-by: Christian Loehle <christian.loehle@arm.com>