[PATCH v2] KVM: x86: Use SRCU to protect zap in __kvm_set_or_clear_apicv_inhibit()

[PATCH v2] KVM: x86: Use SRCU to protect zap in __kvm_set_or_clear_apicv_inhibit()

[Ben Gardon]

kvm_zap_gfn_range() must be called in an SRCU read-critical section, but
there is no SRCU annotation in __kvm_set_or_clear_apicv_inhibit(). This
can lead to the following warning via
kvm_arch_vcpu_ioctl_set_guest_debug() if a Shadow MMU is in use (TDP
MMU disabled or nesting):

[ 1416.659809] =============================
[ 1416.659810] WARNING: suspicious RCU usage
[ 1416.659839] 6.1.0-dbg-DEV #1 Tainted: G S        I
[ 1416.659853] -----------------------------
[ 1416.659854] include/linux/kvm_host.h:954 suspicious rcu_dereference_check() usage!
[ 1416.659856]
...
[ 1416.659904]  dump_stack_lvl+0x84/0xaa
[ 1416.659910]  dump_stack+0x10/0x15
[ 1416.659913]  lockdep_rcu_suspicious+0x11e/0x130
[ 1416.659919]  kvm_zap_gfn_range+0x226/0x5e0
[ 1416.659926]  ? kvm_make_all_cpus_request_except+0x18b/0x1e0
[ 1416.659935]  __kvm_set_or_clear_apicv_inhibit+0xcc/0x100
[ 1416.659940]  kvm_arch_vcpu_ioctl_set_guest_debug+0x350/0x390
[ 1416.659946]  kvm_vcpu_ioctl+0x2fc/0x620
[ 1416.659955]  __se_sys_ioctl+0x77/0xc0
[ 1416.659962]  __x64_sys_ioctl+0x1d/0x20
[ 1416.659965]  do_syscall_64+0x3d/0x80
[ 1416.659969]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Always take the KVM SRCU read lock in __kvm_set_or_clear_apicv_inhibit()
to protect the GFN to memslot translation. The SRCU read lock is not
technically required when no Shadow MMUs are in use, since the TDP MMU
walks the paging structures from the roots and does not need to look up
GFN translations in the memslots, but make the SRCU locking
unconditional for simplicty.

In most cases, the SRCU locking is taken care of in the vCPU run loop,
but when called through the KVM_SET_GUEST_DEBUG IOCTL, the SRCU read
lock is missing.

Tested: ran tools/testing/selftests/kvm/x86_64/debug_regs on a DBG
	build. This patch causes the suspicious RCU warning to disappear.
	Note that the warning is hit in __kvm_zap_rmaps(), so
	kvm_memslots_have_rmaps() must return true in order for this to
	repro (i.e. the TDP MMU must be off or nesting in use.)

Reported-by: Greg Thelen <gthelen@google.com>
Fixes: 36222b117e36 ("KVM: x86: don't disable APICv memslot when inhibited")
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd9eb13e2ed7..6d853e5f683d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10091,7 +10091,10 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
 		kvm->arch.apicv_inhibit_reasons = new;
 		if (new) {
 			unsigned long gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
+			int idx = srcu_read_lock(&kvm->srcu);
+
 			kvm_zap_gfn_range(kvm, gfn, gfn+1);
+			srcu_read_unlock(&kvm->srcu, idx);
 		}
 	} else {
 		kvm->arch.apicv_inhibit_reasons = new;
-- 
2.38.1.431.g37b22c650d-goog

Re: [PATCH v2] KVM: x86: Use SRCU to protect zap in __kvm_set_or_clear_apicv_inhibit()

[Sean Christopherson]

On Wed, Nov 02, 2022, Ben Gardon wrote:
> kvm_zap_gfn_range() must be called in an SRCU read-critical section, but
> there is no SRCU annotation in __kvm_set_or_clear_apicv_inhibit(). This
> can lead to the following warning via
> kvm_arch_vcpu_ioctl_set_guest_debug() if a Shadow MMU is in use (TDP
> MMU disabled or nesting):
> 
> [ 1416.659809] =============================
> [ 1416.659810] WARNING: suspicious RCU usage
> [ 1416.659839] 6.1.0-dbg-DEV #1 Tainted: G S        I
> [ 1416.659853] -----------------------------
> [ 1416.659854] include/linux/kvm_host.h:954 suspicious rcu_dereference_check() usage!
> [ 1416.659856]
> ...
> [ 1416.659904]  dump_stack_lvl+0x84/0xaa
> [ 1416.659910]  dump_stack+0x10/0x15
> [ 1416.659913]  lockdep_rcu_suspicious+0x11e/0x130
> [ 1416.659919]  kvm_zap_gfn_range+0x226/0x5e0
> [ 1416.659926]  ? kvm_make_all_cpus_request_except+0x18b/0x1e0
> [ 1416.659935]  __kvm_set_or_clear_apicv_inhibit+0xcc/0x100
> [ 1416.659940]  kvm_arch_vcpu_ioctl_set_guest_debug+0x350/0x390
> [ 1416.659946]  kvm_vcpu_ioctl+0x2fc/0x620
> [ 1416.659955]  __se_sys_ioctl+0x77/0xc0
> [ 1416.659962]  __x64_sys_ioctl+0x1d/0x20
> [ 1416.659965]  do_syscall_64+0x3d/0x80
> [ 1416.659969]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Always take the KVM SRCU read lock in __kvm_set_or_clear_apicv_inhibit()
> to protect the GFN to memslot translation. The SRCU read lock is not
> technically required when no Shadow MMUs are in use, since the TDP MMU
> walks the paging structures from the roots and does not need to look up
> GFN translations in the memslots, but make the SRCU locking
> unconditional for simplicty.
> 
> In most cases, the SRCU locking is taken care of in the vCPU run loop,
> but when called through the KVM_SET_GUEST_DEBUG IOCTL, the SRCU read
> lock is missing.

Nit, it not just KVM_SET_GUEST_DEBUG.  If it were just KVM_SET_GUEST_DEBUG, I
might have advocated putting the fix KVM_SET_GUEST_DEBUG.

> Tested: ran tools/testing/selftests/kvm/x86_64/debug_regs on a DBG
> 	build. This patch causes the suspicious RCU warning to disappear.
> 	Note that the warning is hit in __kvm_zap_rmaps(), so
> 	kvm_memslots_have_rmaps() must return true in order for this to
> 	repro (i.e. the TDP MMU must be off or nesting in use.)
> 
> Reported-by: Greg Thelen <gthelen@google.com>
> Fixes: 36222b117e36 ("KVM: x86: don't disable APICv memslot when inhibited")
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v2] KVM: x86: Use SRCU to protect zap in __kvm_set_or_clear_apicv_inhibit()

[Paolo Bonzini]

Queued, thanks.

Paolo