Re: [PATCH v2 2/2] KVM: nVMX: avoid losing the posted notification interrupt when exiting L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: nVMX: incorrect memory barriers (smp_wmb/smp_rmb) fail to prevent Store-Load race
- [High] kvm: nVMX: ignored return value in __nested_vmx_vmexit swallows memory failure
--

commit 8287f538318e13160edd8d2bb1ae4a6ae79766ff
Author: Maxim Levitsky <mlevitsk@redhat.com>

KVM: nVMX: avoid losing the posted notification interrupt when exiting L2

This commit runs vmx_complete_nested_posted_interrupt just before a nested
VM exit to prevent losing nested posted interrupt notifications when the
target vCPU is exiting L2. It also adds memory barriers between
vmx_deliver_nested_posted_interrupt and __nested_vmx_vmexit.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 50548e3483181..92d5ade58dc9c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5103,8 +5103,15 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
>  	if (enable_ept && is_pae_paging(vcpu))
>  		vmx_ept_load_pdptrs(vcpu);
>  
> +
>  	leave_guest_mode(vcpu);
>  
> +	/* pairs with barrier in vmx_deliver_nested_posted_interrupt */
> +	smp_wmb();
> +
> +	if (vmx->nested.pi_pending)
> +		vmx_complete_nested_posted_interrupt(vcpu);

[Severity: High]
Does this ignored return value swallow memory failures?

When vmx_complete_nested_posted_interrupt() encounters an error mapping the
virtual APIC page, it calls kvm_handle_memory_failure() to set
vcpu->run->exit_reason to KVM_EXIT_INTERNAL_ERROR and returns -ENXIO to abort
the current VM run.

Since __nested_vmx_vmexit() returns void and ignores this return value, is the
KVM_EXIT_INTERNAL_ERROR silently overwritten by the next VM-exit, preventing
userspace from receiving the memory failure error?

[Severity: High]
Do these memory barriers properly prevent a Store-Load race?

In __nested_vmx_vmexit(), we store to vcpu->arch.hflags (via leave_guest_mode)
and then load vmx->nested.pi_pending.

In vmx_deliver_nested_posted_interrupt(), we load vcpu->arch.hflags (via
is_guest_mode) and then store to vmx->nested.pi_pending.

This is a Store-Load sequence on the receiver and a Load-Store sequence on the
sender.

On x86, smp_wmb() acts only as a compiler barrier and does not prevent the CPU
store buffer from reordering the load of pi_pending before the store to hflags
becomes globally visible. As a result, the target vCPU can still read
pi_pending == false before its exit from guest mode is visible to the sender.
The sender will then see is_guest_mode == true and set pi_pending = true,
leaving the nested posted interrupt lost.

Is a full memory barrier (smp_mb()) required here to prevent Store-Load
reordering?

> +
>  	if (nested_cpu_has_preemption_timer(vmcs12))
>  		hrtimer_cancel(&to_vmx(vcpu)->nested.preemption_timer);
>  

[ ... ]

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index cc75feec05daa..a40791167b10e 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4362,10 +4362,15 @@ static int vmx_deliver_nested_posted_interrupt(struct kvm_vcpu *vcpu,
>  	 */
>  	if (is_guest_mode(vcpu) &&
>  	    vector == vmx->nested.posted_intr_nv) {
> +
> +		/* pairs with barrier in __nested_vmx_vmexit */
> +		smp_rmb();

[Severity: High]
Does an smp_rmb() here correctly pair with the barrier in __nested_vmx_vmexit?

Since the sequence in vmx_deliver_nested_posted_interrupt() involves a load
(is_guest_mode) followed by a store (pi_pending = true), and the sequence in
__nested_vmx_vmexit() is a Store-Load, is a different synchronization primitive
or stronger barrier pairing needed to prevent the race?

> +
>  		/*
>  		 * If a posted intr is not recognized by hardware,
>  		 * we will accomplish it in the next vmentry.
>  		 */
> +
>  		vmx->nested.pi_pending = true;
>  		kvm_make_request(KVM_REQ_EVENT, vcpu);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011001303.723721-1-mlevitsk@redhat.com?part=2