| Message ID | 20221102193020.1091939-1-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Use SRCU to protect zap in __kvm_set_or_clear_apicv_inhibit | expand |
On Wed, Nov 02, 2022, Ben Gardon wrote: > kvm_zap_gfn_range must be called in an SRCU read-critical section, but Please add parantheses when referencing functions, i.e. kvm_zap_gfn_range(). > there is no SRCU annotation in __kvm_set_or_clear_apicv_inhibit. __kvm_set_or_clear_apicv_inhibit() > Add the needed SRCU annotation. It's not an annotation, acquiring SRCU is very much functional code. > 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.) Please provide the stack trace or at least a verbal description of what paths can reach __kvm_set_or_clear_apicv_inhibit() without holding SRCU, i.e. explain why this bug isn't being hit left and right. E.g. Unconditionally take KVM's SRCU lock in __kvm_set_or_clear_apicv_inhibit() when zapping virtual APIC SPTEs. SRCU must be held when zapping SPTEs in shadow MMUs to protect the gfn=>memslot translation (the TDP MMU walks all roots and so doesn't dereference memslots). In most cases, the inhibits are updated during KVM_RUN and so SRCU is already held, but other ioctls() can also modify inhibits and don't acquire SRCU, e.g. KVM_SET_GUEST_DEBUG and KVM_SET_LAPIC. Acquire SRCU unconditionally to avoid playing whack-a-mole, as nesting SRCU locks is safe and this is not a hot path. > Fixes: 36222b117e36 ("KVM: x86: don't disable APICv memslot when inhibited") Reported-by? IIRC this originated in a syzkaller report?
On Wed, Nov 2, 2022 at 12:47 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Nov 02, 2022, Ben Gardon wrote: > > kvm_zap_gfn_range must be called in an SRCU read-critical section, but > > Please add parantheses when referencing functions, i.e. kvm_zap_gfn_range(). > > > there is no SRCU annotation in __kvm_set_or_clear_apicv_inhibit. > > __kvm_set_or_clear_apicv_inhibit() > > > Add the needed SRCU annotation. > > It's not an annotation, acquiring SRCU is very much functional code. Right, totally true. Will correct. > > > 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.) > > Please provide the stack trace or at least a verbal description of what paths > can reach __kvm_set_or_clear_apicv_inhibit() without holding SRCU, i.e. explain > why this bug isn't being hit left and right. > > E.g. > > Unconditionally take KVM's SRCU lock in __kvm_set_or_clear_apicv_inhibit() > when zapping virtual APIC SPTEs. SRCU must be held when zapping SPTEs in > shadow MMUs to protect the gfn=>memslot translation (the TDP MMU walks all > roots and so doesn't dereference memslots). > > In most cases, the inhibits are updated during KVM_RUN and so SRCU is > already held, but other ioctls() can also modify inhibits and don't > acquire SRCU, e.g. KVM_SET_GUEST_DEBUG and KVM_SET_LAPIC. Acquire SRCU > unconditionally to avoid playing whack-a-mole, as nesting SRCU locks is > safe and this is not a hot path. > > > Fixes: 36222b117e36 ("KVM: x86: don't disable APICv memslot when inhibited") > > Reported-by? IIRC this originated in a syzkaller report? This was found on an non-upstream Google kernel by Greg Thelen, but a great point. I'll credit him in v2.
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;
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. Add the needed SRCU annotation. 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.) 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(+)