linux-kernel - Re: [PATCH v2 02/18] KVM: x86: Use __free(put_cpufreq

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aLCOpfNkcQN9P-Wa@google.com>
Date: Thu, 28 Aug 2025 10:15:17 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Zihuan Zhang <zhangzihuan@...inos.cn>
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

On Thu, Aug 28, 2025, Zihuan Zhang wrote:
> > 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);
> > 		}

...

> Yes, this will indeed change the execution order.
> Can you accept that? 

No, because it's buggy.

> 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();

This all feels very forced, in the sense that we have a shiny new tool and are
trying to use it everywhere without thinking critically about whether or not
doing so is actually an improvement.

At a glance, this is literally the only instance in the entire kernel where the
CPU to use is grabbed immediately before the policy.
 
  $ git grep -B 20 cpufreq_cpu_get | grep -e get_cpu -e smp_processor_id
  arch/x86/kvm/x86.c-			cpu = get_cpu();
  drivers/cpufreq/cppc_cpufreq.c-static int cppc_get_cpu_power(struct device *cpu_dev,
  drivers/cpufreq/cppc_cpufreq.c-static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz,
  drivers/cpufreq/mediatek-cpufreq-hw.c-mtk_cpufreq_get_cpu_power(struct device *cpu_dev, unsigned long *uW,

Probably because KVM's usage is rather bizarre and honestly kind of dumb.  But
KVM has had this behavior for 15+ years, so as weird as it is, I'm not inclined
to change it without a really, really strong reason to do so, e.g. to iterate
over all CPUs or something.

So given that this is the only intance of the problem patter, I think it makes
sense to leave KVM as-is, and not spend a bunch of time trying to figure out how
to make KVM's usage play nice with __free().