| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0ihGrXrOhA+1iwwrr6+SzmQ=3xuy5+F+zLgDZdS7brbCQ@mail.gmail.com>
Date: Wed, 16 Apr 2025 14:50:20 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Christian Loehle <christian.loehle@....com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Viresh Kumar <viresh.kumar@...aro.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Mario Limonciello <mario.limonciello@....com>, Vincent Guittot <vincent.guittot@...aro.org>,
Sultan Alsawaf <sultan@...neltoast.com>, Peter Zijlstra <peterz@...radead.org>,
Valentin Schneider <vschneid@...hat.com>, Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v2 5/6] cpufreq: Avoid using inconsistent policy->min and policy->max
On Wed, Apr 16, 2025 at 2:39 PM Christian Loehle
<christian.loehle@....com> wrote:
>
> On 4/15/25 11:04, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Since cpufreq_driver_resolve_freq() can run in parallel with
> > cpufreq_set_policy() and there is no synchronization between them,
> > the former may access policy->min and policy->max while the latter
> > is updating them and it may see intermediate values of them due
> > to the way the update is carried out. Also the compiler is free
> > to apply any optimizations it wants both to the stores in
> > cpufreq_set_policy() and to the loads in cpufreq_driver_resolve_freq()
> > which may result in additional inconsistencies.
> >
> > To address this, use WRITE_ONCE() when updating policy->min and
> > policy->max in cpufreq_set_policy() and use READ_ONCE() for reading
> > them in cpufreq_driver_resolve_freq(). Moreover, rearrange the update
> > in cpufreq_set_policy() to avoid storing intermediate values in
> > policy->min and policy->max with the help of the observation that
> > their new values are expected to be properly ordered upfront.
> >
> > Also modify cpufreq_driver_resolve_freq() to take the possible reverse
> > ordering of policy->min and policy->max, which may happen depending on
> > the ordering of operations when this function and cpufreq_set_policy()
> > run concurrently, into account by always honoring the max when it
> > turns out to be less than the min (in case it comes from thermal
> > throttling or similar).
> >
> > Fixes: 151717690694 ("cpufreq: Make policy min/max hard requirements")
> > Cc: 5.16+ <stable@...r.kernel.org> # 5.16+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Just so I understand, the reason you don't squish 4-6 into one is
> because this is the only fix? I do get that, but doesn't the fact
> that it could easily be picked for backports make up for the additional
> refactor?
Yeah, I think I'll just merge them together and resend.
> Actual changes from patches 4-6 look good to me.
OK, thanks!
> > ---
> >
> > v1 -> v2: Minor edit in the subject
> >
> > ---
> > drivers/cpufreq/cpufreq.c | 46 ++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 36 insertions(+), 10 deletions(-)
> >
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -490,14 +490,12 @@
> > }
> > EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
> >
> > -static unsigned int clamp_and_resolve_freq(struct cpufreq_policy *policy,
> > - unsigned int target_freq,
> > - unsigned int relation)
> > +static unsigned int __resolve_freq(struct cpufreq_policy *policy,
> > + unsigned int target_freq,
> > + unsigned int relation)
> > {
> > unsigned int idx;
> >
> > - target_freq = clamp_val(target_freq, policy->min, policy->max);
> > -
> > if (!policy->freq_table)
> > return target_freq;
> >
> > @@ -507,6 +505,15 @@
> > return policy->freq_table[idx].frequency;
> > }
> >
> > +static unsigned int clamp_and_resolve_freq(struct cpufreq_policy *policy,
> > + unsigned int target_freq,
> > + unsigned int relation)
> > +{
> > + target_freq = clamp_val(target_freq, policy->min, policy->max);
> > +
> > + return __resolve_freq(policy, target_freq, relation);
> > +}
> > +
> > /**
> > * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported
> > * one.
> > @@ -521,7 +528,22 @@
> > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> > unsigned int target_freq)
> > {
> > - return clamp_and_resolve_freq(policy, target_freq, CPUFREQ_RELATION_LE);
> > + unsigned int min = READ_ONCE(policy->min);
> > + unsigned int max = READ_ONCE(policy->max);
> > +
> > + /*
> > + * If this function runs in parallel with cpufreq_set_policy(), it may
> > + * read policy->min before the update and policy->max after the update
> > + * or the other way around, so there is no ordering guarantee.
> > + *
> > + * Resolve this by always honoring the max (in case it comes from
> > + * thermal throttling or similar).
> > + */
> > + if (unlikely(min > max))
> > + min = max;
> > +
> > + return __resolve_freq(policy, clamp_val(target_freq, min, max),
> > + CPUFREQ_RELATION_LE);
> > }
> > EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
> >
> > @@ -2632,11 +2654,15 @@
> > * Resolve policy min/max to available frequencies. It ensures
> > * no frequency resolution will neither overshoot the requested maximum
> > * nor undershoot the requested minimum.
> > + *
> > + * Avoid storing intermediate values in policy->max or policy->min and
> > + * compiler optimizations around them because them may be accessed
>
> s/them/they/
Yup, thanks!
> > + * concurrently by cpufreq_driver_resolve_freq() during the update.
> > */
> > - policy->min = new_data.min;
> > - policy->max = new_data.max;
> > - policy->min = clamp_and_resolve_freq(policy, policy->min, CPUFREQ_RELATION_L);
> > - policy->max = clamp_and_resolve_freq(policy, policy->max, CPUFREQ_RELATION_H);
> > + WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max, CPUFREQ_RELATION_H));
> > + new_data.min = __resolve_freq(policy, new_data.min, CPUFREQ_RELATION_L);
> > + WRITE_ONCE(policy->min, new_data.min > policy->max ? policy->max : new_data.min);
> > +
> > trace_cpu_frequency_limits(policy);
> >
> > cpufreq_update_pressure(policy);
Thanks for all the reviews!
Powered by blists - more mailing lists