linux-kernel - Re: [PATCH 6.1.y 2/2 V2] KVM: x86: Fix lapic timer interrupt lost after loading a snapshot.

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: <2024082759-theatrics-sulk-85f2@gregkh>
Date: Tue, 27 Aug 2024 15:10:10 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: David Hunter <david.hunter.linux@...il.com>
Cc: seanjc@...gle.com, dave.hansen@...ux.intel.com, hpa@...or.com,
	javier.carrasco.cruz@...il.com, jmattson@...gle.com,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	lirongqing@...du.com, pbonzini@...hat.com, pshier@...gle.com,
	shuah@...nel.org, stable@...r.kernel.org, x86@...nel.org,
	Haitao Shan <hshan@...gle.com>
Subject: Re: [PATCH 6.1.y 2/2 V2] KVM: x86: Fix lapic timer interrupt lost
 after loading a snapshot.

On Mon, Aug 26, 2024 at 06:13:36PM -0400, David Hunter wrote:
> 
> [ Upstream Commit 9cfec6d097c607e36199cf0cfbb8cf5acbd8e9b2]

This is already in the 6.1.66 release, so do you want it applied again?

> From: Haitao Shan <hshan@...gle.com>
> Date:   Tue Sep 12 16:55:45 2023 -0700 
> 
> When running android emulator (which is based on QEMU 2.12) on
> certain Intel hosts with kernel version 6.3-rc1 or above, guest
> will freeze after loading a snapshot. This is almost 100%
> reproducible. By default, the android emulator will use snapshot
> to speed up the next launching of the same android guest. So
> this breaks the android emulator badly.
> 
> I tested QEMU 8.0.4 from Debian 12 with an Ubuntu 22.04 guest by
> running command "loadvm" after "savevm". The same issue is
> observed. At the same time, none of our AMD platforms is impacted.
> More experiments show that loading the KVM module with
> "enable_apicv=false" can workaround it.
> 
> The issue started to show up after commit 8e6ed96cdd50 ("KVM: x86:
> fire timer when it is migrated and expired, and in oneshot mode").
> However, as is pointed out by Sean Christopherson, it is introduced
> by commit 967235d32032 ("KVM: vmx: clear pending interrupts on
> KVM_SET_LAPIC"). commit 8e6ed96cdd50 ("KVM: x86: fire timer when
> it is migrated and expired, and in oneshot mode") just makes it
> easier to hit the issue.
> 
> Having both commits, the oneshot lapic timer gets fired immediately
> inside the KVM_SET_LAPIC call when loading the snapshot. On Intel
> platforms with APIC virtualization and posted interrupt processing,
> this eventually leads to setting the corresponding PIR bit. However,
> the whole PIR bits get cleared later in the same KVM_SET_LAPIC call
> by apicv_post_state_restore. This leads to timer interrupt lost.
> 
> The fix is to move vmx_apicv_post_state_restore to the beginning of
> the KVM_SET_LAPIC call and rename to vmx_apicv_pre_state_restore.
> What vmx_apicv_post_state_restore does is actually clearing any
> former apicv state and this behavior is more suitable to carry out
> in the beginning.
> 
> Fixes: 967235d32032 ("KVM: vmx: clear pending interrupts on KVM_SET_LAPIC")
> Cc: stable@...r.kernel.org
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Haitao Shan <hshan@...gle.com>
> Link: https://lore.kernel.org/r/20230913000215.478387-1-hshan@google.com
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> 
> (Cherry-Picked from commit 9cfec6d097c607e36199cf0cfbb8cf5acbd8e9b2)
> Signed-off-by: David Hunter <david.hunter.linux@...il.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 87abf4eebf8a..4040075bbd5a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8203,6 +8203,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
>  	.load_eoi_exitmap = vmx_load_eoi_exitmap,
>  	.apicv_pre_state_restore = vmx_apicv_pre_state_restore,
>  	.check_apicv_inhibit_reasons = vmx_check_apicv_inhibit_reasons,
> +	.required_apicv_inhibits = VMX_REQUIRED_APICV_INHIBITS,
>  	.hwapic_irr_update = vmx_hwapic_irr_update,
>  	.hwapic_isr_update = vmx_hwapic_isr_update,
>  	.guest_apic_has_interrupt = vmx_guest_apic_has_interrupt,

Wait, this is just one hunk?  This feels wrong, you didn't say why you
modfied this from the original commit, or backport, what was wrong with
that?

thanks,

greg k-h