linux-kernel - Re: [PATCH v2] KVM: x86: Unify L1TF flushing under per-CPU variable

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] [day] [month] [year] [list]

Message-ID: <DDJWE0C6OM45.2AKEJR1EG8Z6Q@google.com>
Date: Thu, 16 Oct 2025 16:47:02 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>, Brendan Jackman <jackmanb@...gle.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>, 
	"H. Peter Anvin" <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, <linux-kernel@...r.kernel.org>, 
	<kvm@...r.kernel.org>
Subject: Re: [PATCH v2] KVM: x86: Unify L1TF flushing under per-CPU variable

On Thu Oct 16, 2025 at 4:41 PM UTC, Sean Christopherson wrote:
> On Thu, Oct 16, 2025, Brendan Jackman wrote:
>> On Thu Oct 16, 2025 at 3:50 PM UTC, Sean Christopherson wrote:
>> > On Wed, Oct 15, 2025, Brendan Jackman wrote:
>> >> Currently the tracking of the need to flush L1D for L1TF is tracked by
>> >> two bits: one per-CPU and one per-vCPU.
>> >> 
>> >> The per-vCPU bit is always set when the vCPU shows up on a core, so
>> >> there is no interesting state that's truly per-vCPU. Indeed, this is a
>> >> requirement, since L1D is a part of the physical CPU.
>> >> 
>> >> So simplify this by combining the two bits.
>> >> 
>> >> The vCPU bit was being written from preemption-enabled regions. For
>> >> those cases, use raw_cpu_write() (via a variant of the setter function)
>> >> to avoid DEBUG_PREEMPT failures. If the vCPU is getting migrated, the
>> >> CPU that gets its bit set in these paths is not important; vcpu_load()
>> >> must always set it on the destination CPU before the guest is resumed.
>> >> 
>> >> Signed-off-by: Brendan Jackman <jackmanb@...gle.com>
>> >> ---
>> >
>> > ...
>> >
>> >> @@ -78,6 +79,11 @@ static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void)
>> >>  	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
>> >>  }
>> >>  
>> >> +static __always_inline void kvm_set_cpu_l1tf_flush_l1d_raw(void)
>> >> +{
>> >> +	raw_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
>> >> +}
>> >
>> > TL;DR: I'll post a v3 with a slightly tweaked version of this patch at the end.
>> >
>> > Rather than add a "raw" variant, I would rather have a wrapper in arch/x86/kvm/x86.h
>> > that disables preemption, with a comment explaining why it's ok to enable preemption
>> > after setting the per-CPU flag.  Without such a comment, choosing between the two
>> > variants looks entirely random
>> >
>> > Alternatively, all writes could be raw, but that
>> > feels wrong/weird, and in practice disabling preemption in the relevant paths is a
>> > complete non-issue.
>> 
>> Hm, why does making every write _raw feel weird but adding
>> preempt_disable() to every write doesn't? Both feel equally weird to me.
>
> I completely agree that both approaches are odd/weird.
>
>> But the latter has the additional weirdness of using preempt_disable()
>> as a way to signal "I know what I'm doing", when that signal is already
>> explicitly documented as the purpose of raw_cpu_write().
>
> True.  Aha!
>
> With the #ifdefs in place, KVM doesn't need arch/x86/include/asm/hardirq.h to
> provide a wrapper.  irq_stat is already exported, the wrapper exists purely so
> that kvm_set_cpu_l1tf_flush_l1d() can be invoked without callers having to check
> CONFIG_KVM_INTEL.
>
> Not yet tested, but how about this?
>
> static __always_inline void kvm_request_l1tf_flush_l1d(void)
> {
> #if IS_ENABLED(CONFIG_CPU_MITIGATIONS) && IS_ENABLED(CONFIG_KVM_INTEL)
> 	/*
> 	 * Use a raw write to set the per-CPU flag, as KVM will ensure a flush
> 	 * even if preemption is currently enabled..  If the current vCPU task
> 	 * is migrated to a different CPU (or userspace runs the vCPU on a
> 	 * different task) before the next VM-Entry, then kvm_arch_vcpu_load()
> 	 * will request a flush on the new CPU.
> 	 */
> 	raw_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
> #endif
> }

Yeah, just poking irq_stat directly seems fine to me.