| 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: <874d821e-8ea3-40ac-921b-c19bb380a456@kylinos.cn>
Date: Thu, 28 Aug 2025 09:17:59 +0800
From: Zihuan Zhang <zhangzihuan@...inos.cn>
To: Sean Christopherson <seanjc@...gle.com>
Cc: "Rafael J . wysocki" <rafael@...nel.org>,
Viresh Kumar <viresh.kumar@...aro.org>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, Markus Mayer
<mmayer@...adcom.com>, Florian Fainelli <florian.fainelli@...adcom.com>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Madhavan Srinivasan <maddy@...ux.ibm.com>,
Michael Ellerman <mpe@...erman.id.au>, Krzysztof Kozlowski
<krzk@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>, Tvrtko Ursulin
<tursulin@...ulin.net>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Daniel Lezcano <daniel.lezcano@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>, Shawn Guo <shawnguo@...nel.org>,
Eduardo Valentin <edubezval@...il.com>, Keerthy <j-keerthy@...com>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
zhenglifeng <zhenglifeng1@...wei.com>, "H . Peter Anvin" <hpa@...or.com>,
Zhang Rui <rui.zhang@...el.com>, Len Brown <lenb@...nel.org>,
Nicholas Piggin <npiggin@...il.com>,
Christophe Leroy <christophe.leroy@...roup.eu>,
Lukasz Luba <lukasz.luba@....com>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Beata Michalska <beata.michalska@....com>, Fabio Estevam
<festevam@...il.com>, Pavel Machek <pavel@...nel.org>,
Sumit Gupta <sumitg@...dia.com>,
Prasanna Kumar T S M <ptsm@...ux.microsoft.com>,
Sudeep Holla <sudeep.holla@....com>, Yicong Yang <yangyicong@...ilicon.com>,
linux-pm@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
linux-acpi@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
linux-samsung-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-tegra@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, imx@...ts.linux.dev,
linux-omap@...r.kernel.org, linux-mediatek@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/18] KVM: x86: Use __free(put_cpufreq_policy) for
policy reference
Hi,
在 2025/8/27 22:13, Sean Christopherson 写道:
> On Wed, Aug 27, 2025, Zihuan Zhang wrote:
>> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
>> annotation for policy references. This reduces the risk of reference
>> counting mistakes and aligns the code with the latest kernel style.
>>
>> No functional change intended.
>>
>> Signed-off-by: Zihuan Zhang <zhangzihuan@...inos.cn>
>> ---
>> arch/x86/kvm/x86.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index a1c49bc681c4..2a825f4ec701 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -9492,16 +9492,14 @@ static void kvm_timer_init(void)
>> max_tsc_khz = tsc_khz;
>>
>> if (IS_ENABLED(CONFIG_CPU_FREQ)) {
>> - struct cpufreq_policy *policy;
>> + struct cpufreq_policy *policy __free(put_cpufreq_policy);
>> int cpu;
>>
>> cpu = get_cpu();
>> policy = cpufreq_cpu_get(cpu);
>> - if (policy) {
>> - if (policy->cpuinfo.max_freq)
>> - max_tsc_khz = policy->cpuinfo.max_freq;
>> - cpufreq_cpu_put(policy);
>> - }
>> + if (policy && policy->cpuinfo.max_freq)
>> + max_tsc_khz = policy->cpuinfo.max_freq;
>> +
>> put_cpu();
> Hmm, this is technically buggy. __free() won't invoke put_cpufreq_policy() until
> policy goes out of scope, and so using __free() means the code is effectively:
>
> if (IS_ENABLED(CONFIG_CPU_FREQ)) {
> struct cpufreq_policy *policy;
> int cpu;
>
> cpu = get_cpu();
> policy = cpufreq_cpu_get(cpu);
> if (policy && policy->cpuinfo.max_freq)
> max_tsc_khz = policy->cpuinfo.max_freq;
> put_cpu();
>
> if (policy)
> cpufreq_cpu_put(policy);
> }
>
> That's "fine" because the policy isn't truly referenced after preemption is
> disabled, the lifecycle of the policy doesn't rely on preemption being disabled,
> and KVM doesn't actually care which CPU is used to get the max frequency, i.e.
> this would technically be "fine" too:
>
> if (IS_ENABLED(CONFIG_CPU_FREQ)) {
> struct cpufreq_policy *policy;
> int cpu;
>
> cpu = get_cpu();
> policy = cpufreq_cpu_get(cpu);
> put_cpu();
>
> if (policy && policy->cpuinfo.max_freq)
> max_tsc_khz = policy->cpuinfo.max_freq;
>
> if (policy)
> cpufreq_cpu_put(policy);
> }
>
> But given that the code we have today is perfectly readable, I don't see any
> reason to switch to __free() given that's it's technically flawed. So I'm very
> strongly inclined to skip this patch and keep things as-is.
Yes, this will indeed change the execution order.
Can you accept that? Personally, I don’t think it’s ideal either.
if (IS_ENABLED(CONFIG_CPU_FREQ)) {
int cpu;
cpu = get_cpu();
{
struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
if (policy && policy->cpuinfo.max_freq)
max_tsc_khz = policy->cpuinfo.max_freq;
}
put_cpu();
}
Other places may also have the same issue,
maybe we should consider introducing a macro to handle this properly,
so that initialization and cleanup are well defined without changing
the existing order unexpected.
like this:
#define WITH_CPUFREQ_POLICY(cpu) {\
for(struct cpufreq_policy *policy __free(put_cpufreq_policy) = \
cpufreq_cpu_get(cpu); \
policy;)
Then Use it:
if (IS_ENABLED(CONFIG_CPU_FREQ)) {
int cpu;
cpu = get_cpu();
WITH_CPUFREQ_POLICY(cpu){
if (policy->cpuinfo.max_freq)
max_tsc_khz = policy->cpuinfo.max_freq;
}
put_cpu();
}
Powered by blists - more mailing lists