Re: [PATCH 11/17] KVM: nVMX: Use pinned pfncache for L1 APIC virtualization pages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: nVMX: Guest-triggered KVM_EXIT_INTERNAL_ERROR via unaligned APIC-access address
- [Medium] kvm: lockdep: AB-BA circular dependency between gpc->refresh_lock and kvm->gpc_srcu
--

commit daeb1861c76d9216ee8842648d7c50ed244624b3
Author: Fred Griffoul <fgriffo@xxxxxxxxxxxx>
Date:   Sun Sep 20 20:49:39 2026 +0000

    KVM: nVMX: Use pinned pfncache for L1 APIC virtualization pages

    Replace kvm_host_map usage with gfn_to_pfn_cache for the L1 pages whose
    physical addresses are handed to the CPU in vmcs02 for direct use while
    running L2. This eliminates the map/unmap cycle on every L2 VM-entry/exit.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 5fe7e5d1f72d9..105ff6cbfe583 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c

[ ... ]

> @@ -3494,26 +3591,74 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>  
>  
>  	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) {
> -		map = &vmx->nested.apic_access_page_map;
> -
> -		if (!kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->apic_access_addr), map)) {
> -			vmcs_write64(APIC_ACCESS_ADDR, pfn_to_hpa(map->pfn));
> -		} else {
> +		int err = nested_gpc_hpa(&vmx->nested.apic_access_page_cache,
> +					 vmcs12->apic_access_addr, &hpa);
> +		if (!err) {
> +			vmcs_write64(APIC_ACCESS_ADDR, hpa);
> +		} else if (err == -EAGAIN) {
> +			kvm_gpc_unpin_for_guest(&vmx->nested.apic_access_page_cache);
> +			kvm_gpc_deactivate(&vmx->nested.apic_access_page_cache);
> +			goto retry;
> +		} else if (!lazy) {
> +			/*
> +			 * Entry-time resolution (VMLAUNCH/VMRESUME, or resume
> +			 * from KVM_SET_NESTED_STATE): an unbackable
> +			 * APIC-access page is a misconfiguration to report,
> +			 * not a transient to ride out.
> +			 */
>  			pr_debug_ratelimited("%s: no backing for APIC-access address in vmcs12\n",
>  					     __func__);
>  			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;

[Severity: High]
Does this allow an untrusted L1 guest to crash the VM by triggering a
KVM_EXIT_INTERNAL_ERROR?

The L1 guest can write a non-page-aligned address to the APIC_ACCESS_ADDR field
in vmcs12 and execute VMLAUNCH. When nested_get_vmcs12_pages() calls
nested_gpc_hpa() with this raw, unaligned address, nested_gpc_lock() will return
-EINVAL due to its strict !PAGE_ALIGNED(gpa) check.

Because this non-lazy path treats -EINVAL as an unbackable page and aborts the
entry with KVM_EXIT_INTERNAL_ERROR, the host userspace might kill the entire VM.
Should this be handled as a graceful architectural VM-Entry failure instead?

>  			vcpu->run->internal.suberror =
>  				KVM_INTERNAL_ERROR_EMULATION;

[ ... ]

> @@ -4065,6 +4269,8 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
>  static int vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	int pi_idx, vapic_idx;
> +	struct pi_desc *pi_desc;
>  	int max_irr;
>  	void *vapic_page;
>  	u16 status;
> @@ -4072,22 +4278,32 @@ static int vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
>  	if (!vmx->nested.pi_pending)
>  		return 0;
>  
> -	if (!vmx->nested.pi_desc)
> +	pi_idx = nested_gpc_lock_if_active(&vmx->nested.pi_desc_cache);
> +	if (pi_idx < 0)
>  		goto mmio_needed;
>  
> +	pi_desc = nested_pi_desc(vmx);
>  	vmx->nested.pi_pending = false;
>  
> -	if (!pi_test_and_clear_on(vmx->nested.pi_desc))
> +	if (!pi_test_and_clear_on(pi_desc)) {
> +		nested_gpc_unlock(&vmx->nested.pi_desc_cache, pi_idx);
>  		return 0;
> +	}
>  
> -	max_irr = pi_find_highest_vector(vmx->nested.pi_desc);
> +	max_irr = pi_find_highest_vector(pi_desc);
>  	if (max_irr > 0) {
> -		vapic_page = vmx->nested.virtual_apic_map.hva;
> -		if (!vapic_page)
> +		vapic_idx = nested_gpc_lock_if_active(&vmx->nested.virtual_apic_cache);

[Severity: Medium]
Will this nested lock cause an AB-BA circular locking dependency warning from
lockdep?

Here in vmx_complete_nested_posted_interrupt(), nested_gpc_lock_if_active() is
first called for pi_desc_cache, acquiring the SRCU read lock for kvm->gpc_srcu.
Then, while still holding this SRCU read lock, it attempts to lock the
virtual_apic_cache. If a refresh is needed, kvm_gpc_refresh() is called, which
acquires the gpc->refresh_lock mutex.

Since all gpc->refresh_lock instances share the same lock class, lockdep will
observe gpc_srcu -> refresh_lock here.

However, in kvm_gpc_deactivate() in virt/kvm/pfncache.c:

	guard(mutex)(&gpc->refresh_lock);
	if (gpc->active) {
		...
		if (old_state & (GPC_VALID | GPC_INVALIDATING))
			synchronize_srcu_atomic(&kvm->gpc_srcu);

The refresh_lock is held while synchronize_srcu_atomic() waits, establishing the
reverse dependency: refresh_lock -> gpc_srcu. This circular dependency can
disable lockdep and pollute kernel logs.

> +		if (vapic_idx < 0) {
> +			nested_gpc_unlock(&vmx->nested.pi_desc_cache, pi_idx);
>  			goto mmio_needed;
> +		}
> +		vapic_page = vmx->nested.virtual_apic_cache.khva;
> +
> +		__kvm_apic_update_irr(pi_desc->pir, vapic_page, &max_irr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920211920.928306-1-dwmw2@xxxxxxxxxxxxx?part=11