| 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: <CAJZ5v0hTbdTaos9gc8ubPNdjq+K-pBwYpAvT8Rm887EZpSm1bw@mail.gmail.com>
Date: Sun, 27 Apr 2025 13:41:36 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>,
Christian Loehle <christian.loehle@....com>, LKML <linux-kernel@...r.kernel.org>,
Viresh Kumar <viresh.kumar@...aro.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Mario Limonciello <mario.limonciello@....com>, Sultan Alsawaf <sultan@...neltoast.com>,
Stephan Gerhold <stephan.gerhold@...aro.org>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v2] cpufreq: Fix setting policy limits when frequency
tables are used
On Sun, Apr 27, 2025 at 4:26 AM zhenglifeng (A) <zhenglifeng1@...wei.com> wrote:
>
> On 2025/4/25 19:36, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Commit 7491cdf46b5c ("cpufreq: Avoid using inconsistent policy->min and
> > policy->max") overlooked the fact that policy->min and policy->max were
> > accessed directly in cpufreq_frequency_table_target() and in the
> > functions called by it. Consequently, the changes made by that commit
> > led to problems with setting policy limits.
> >
> > Address this by passing the target frequency limits to __resolve_freq()
> > and cpufreq_frequency_table_target() and propagating them to the
> > functions called by the latter.
> >
> > Fixes: 7491cdf46b5c ("cpufreq: Avoid using inconsistent policy->min and policy->max")
> > Link: https://lore.kernel.org/linux-pm/aAplED3IA_J0eZN0@linaro.org/
> > Reported-by: Stephan Gerhold <stephan.gerhold@...aro.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >
> > The v1 is here: https://lore.kernel.org/linux-pm/12665363.O9o76ZdvQC@rjwysocki.net/
> >
> > v1 -> v2:
> > * Do clamp_val(target_freq, min, max) before checking freq_table against
> > NULL in __resolve_freq().
> > * Update comment in cpufreq_frequency_table_target() to match the new code.
> >
> > ---
> > drivers/cpufreq/cpufreq.c | 22 ++++++---
> > drivers/cpufreq/cpufreq_ondemand.c | 3 -
> > drivers/cpufreq/freq_table.c | 6 +-
> > include/linux/cpufreq.h | 83 ++++++++++++++++++++++++-------------
> > 4 files changed, 73 insertions(+), 41 deletions(-)
> >
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -491,14 +491,18 @@
> > EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
> >
> > static unsigned int __resolve_freq(struct cpufreq_policy *policy,
> > - unsigned int target_freq, unsigned int relation)
> > + unsigned int target_freq,
> > + unsigned int min, unsigned int max,
> > + unsigned int relation)
> > {
> > unsigned int idx;
> >
> > + target_freq = clamp_val(target_freq, min, max);
> > +
> > if (!policy->freq_table)
> > return target_freq;
> >
> > - idx = cpufreq_frequency_table_target(policy, target_freq, relation);
> > + idx = cpufreq_frequency_table_target(policy, target_freq, min, max, relation);
> > policy->cached_resolved_idx = idx;
> > policy->cached_target_freq = target_freq;
> > return policy->freq_table[idx].frequency;
> > @@ -532,8 +536,7 @@
> > if (unlikely(min > max))
> > min = max;
> >
> > - return __resolve_freq(policy, clamp_val(target_freq, min, max),
> > - CPUFREQ_RELATION_LE);
> > + return __resolve_freq(policy, target_freq, min, max, CPUFREQ_RELATION_LE);
> > }
> > EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
> >
> > @@ -2351,8 +2354,8 @@
> > if (cpufreq_disabled())
> > return -ENODEV;
> >
> > - target_freq = clamp_val(target_freq, policy->min, policy->max);
> > - target_freq = __resolve_freq(policy, target_freq, relation);
> > + target_freq = __resolve_freq(policy, target_freq, policy->min,
> > + policy->max, relation);
> >
> > pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n",
> > policy->cpu, target_freq, relation, old_target_freq);
> > @@ -2650,8 +2653,11 @@
> > * compiler optimizations around them because they may be accessed
> > * concurrently by cpufreq_driver_resolve_freq() during the update.
> > */
> > - 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->max, __resolve_freq(policy, new_data.max,
> > + new_data.min, new_data.max,
> > + CPUFREQ_RELATION_H));
> > + new_data.min = __resolve_freq(policy, new_data.min, new_data.min,
> > + new_data.max, CPUFREQ_RELATION_L);
> > WRITE_ONCE(policy->min, new_data.min > policy->max ? policy->max : new_data.min);
>
> It might be better like:
>
> - 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);
> + WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max,
> + new_data.min, new_data.max,
> + CPUFREQ_RELATION_H));
> + WRITE_ONCE(policy->min, __resolve_freq(policy, new_data.min,
> + new_data.min, policy->max,
> + CPUFREQ_RELATION_L));
>
Not really because policy->max may be less than new_data.min at this
point AFAICS.
Powered by blists - more mailing lists