cpufreq: schedutil: Publish util hooks only after all sg_cpu are initialized

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

Posted by Zhongqiu Han 1 week, 2 days ago

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;
 }
 
-- 
2.43.0

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

Posted by Christian Loehle 1 week, 2 days ago

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>

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

Posted by Rafael J. Wysocki (Intel) 3 days, 2 hours ago

On Thu, Jul 16, 2026 at 2:25 PM Christian Loehle
<christian.loehle@arm.com> wrote:
>
> 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>

Applied as 7.2-rc material, thanks!