| 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: <41bfb025-008c-db03-2f6d-33b2d542ae65@amd.com>
Date: Wed, 26 Mar 2025 10:34:03 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org,
Paolo Bonzini <pbonzini@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>, Michael Roth <michael.roth@....com>
Subject: Re: [PATCH] KVM: SVM: Fix SNP AP destroy race with VMRUN
On 3/25/25 12:49, Tom Lendacky wrote:
> On 3/21/25 18:17, Sean Christopherson wrote:
>> On Fri, Mar 21, 2025, Tom Lendacky wrote:
>>> On 3/18/25 08:47, Tom Lendacky wrote:
>>>> On 3/18/25 07:43, Tom Lendacky wrote:
>>>>>>> Very off-the-cuff, but I assume KVM_REQ_UPDATE_PROTECTED_GUEST_STATE just needs
>>>>>>> to be annotated with KVM_REQUEST_WAIT.
>>>>>>
>>>>>> Ok, nice. I wasn't sure if KVM_REQUEST_WAIT would be appropriate here.
>>>>>> This is much simpler. Let me test it out and resend if everything goes ok.
>>>>>
>>>>> So that doesn't work. I can still get an occasional #VMEXIT_INVALID. Let
>>>>> me try to track down what is happening with this approach...
>>>>
>>>> Looks like I need to use kvm_make_vcpus_request_mask() instead of just a
>>>> plain kvm_make_request() followed by a kvm_vcpu_kick().
>>
>> Ugh, I was going to say "you don't need to do that", but I forgot that
>> kvm_vcpu_kick() subtly doesn't honor KVM_REQUEST_WAIT.
>>
>> Ooof, I'm 99% certain that's causing bugs elsewhere. E.g. arm64's KVM_REQ_SLEEP
>> uses the same "broken" pattern (LOL, which means that of course RISC-V does too).
>> In quotes, because kvm_vcpu_kick() is the one that sucks.
>>
>> I would rather fix that a bit more directly and obviously. IMO, converting to
>> smp_call_function_single() isntead of bastardizing smp_send_reschedule() is worth
>> doing regardless of the WAIT mess. This will allow cleaning up a bunch of
>> make_request+kick pairs, it'll just take a bit of care to make sure we don't
>> create a WAIT where one isn't wanted (though those probably should have a big fat
>> comment anyways).
>>
>> Compiled tested only.
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 5de20409bcd9..fd9d9a3ee075 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1505,7 +1505,16 @@ bool kvm_vcpu_block(struct kvm_vcpu *vcpu);
>> void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
>> void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
>> bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
>> -void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
>> +
>> +#ifndef CONFIG_S390
>> +void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait);
>> +
>> +static inline void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> +{
>> + __kvm_vcpu_kick(vcpu, false);
>> +}
>> +#endif
>> +
>> int kvm_vcpu_yield_to(struct kvm_vcpu *target);
>> void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu, bool yield_to_kernel_mode);
>>
>> @@ -2253,6 +2262,14 @@ static __always_inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
>> __kvm_make_request(req, vcpu);
>> }
>>
>> +#ifndef CONFIG_S390
>> +static inline void kvm_make_request_and_kick(int req, struct kvm_vcpu *vcpu)
>> +{
>> + kvm_make_request(req, vcpu);
>> + __kvm_vcpu_kick(vcpu, req & KVM_REQUEST_WAIT);
>> +}
>> +#endif
>> +
>> static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
>> {
>> return READ_ONCE(vcpu->requests);
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 201c14ff476f..2a5120e2e6b4 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3734,7 +3734,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
>> /*
>> * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
>> */
>> -void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> +void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait)
>> {
>> int me, cpu;
>>
>> @@ -3764,12 +3764,12 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> if (kvm_arch_vcpu_should_kick(vcpu)) {
>> cpu = READ_ONCE(vcpu->cpu);
>> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>> - smp_send_reschedule(cpu);
>> + smp_call_function_single(cpu, ack_kick, NULL, wait);
>
> In general, this approach works. However, this change triggered
>
> WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> && !oops_in_progress);
>
> in kernel/smp.c.
Is keeping the old behavior desirable when IRQs are disabled? Something
like:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a6fedcadd036..81cbc55eac3a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3754,8 +3754,14 @@ void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait)
*/
if (kvm_arch_vcpu_should_kick(vcpu)) {
cpu = READ_ONCE(vcpu->cpu);
- if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
- smp_call_function_single(cpu, ack_kick, NULL, wait);
+ if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
+ WARN_ON_ONCE(wait && irqs_disabled());
+
+ if (irqs_disabled())
+ smp_send_reschedule(cpu);
+ else
+ smp_call_function_single(cpu, ack_kick, NULL, wait);
+ }
}
out:
put_cpu();
>
> Call path was:
> WARNING: CPU: 13 PID: 3467 at kernel/smp.c:652 smp_call_function_single+0x100/0x120
> ...
>
> Call Trace:
> <TASK>
> ? show_regs+0x69/0x80
> ? __warn+0x8d/0x130
> ? smp_call_function_single+0x100/0x120
> ? report_bug+0x182/0x190
> ? handle_bug+0x63/0xa0
> ? exc_invalid_op+0x19/0x70
> ? asm_exc_invalid_op+0x1b/0x20
> ? __pfx_ack_kick+0x10/0x10 [kvm]
> ? __pfx_ack_kick+0x10/0x10 [kvm]
> ? smp_call_function_single+0x100/0x120
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? migrate_folio_done+0x7f/0x90
> __kvm_vcpu_kick+0xa1/0xb0 [kvm]
> svm_complete_interrupt_delivery+0x93/0xa0 [kvm_amd]
> svm_deliver_interrupt+0x3e/0x50 [kvm_amd]
> __apic_accept_irq+0x17f/0x2a0 [kvm]
> kvm_irq_delivery_to_apic_fast+0x149/0x1b0 [kvm]
> kvm_arch_set_irq_inatomic+0x9b/0xd0 [kvm]
> irqfd_wakeup+0xf4/0x230 [kvm]
> ? __pfx_kvm_set_msi+0x10/0x10 [kvm]
> __wake_up_common+0x7b/0xa0
> __wake_up_locked_key+0x18/0x20
> eventfd_write+0xbe/0x1d0
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? security_file_permission+0x134/0x150
> vfs_write+0xfb/0x3f0
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __handle_mm_fault+0x930/0x1040
> ksys_write+0x6a/0xe0
> __x64_sys_write+0x19/0x20
> x64_sys_call+0x16af/0x2140
> do_syscall_64+0x6b/0x110
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? count_memcg_events.constprop.0+0x1e/0x40
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? handle_mm_fault+0x18c/0x270
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? irqentry_exit_to_user_mode+0x2f/0x170
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? irqentry_exit+0x1d/0x30
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? exc_page_fault+0x89/0x160
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Thanks,
> Tom
>
>> }
>> out:
>> put_cpu();
>> }
>> -EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
>> +EXPORT_SYMBOL_GPL(__kvm_vcpu_kick);
>> #endif /* !CONFIG_S390 */
>>
>> int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>>
Powered by blists - more mailing lists