On Wed, 31 Mar 2021 at 01:01, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
> pvclock_gtod_sync_lock can be taken with interrupts disabled if the
> preempt notifier calls get_kvmclock_ns to update the Xen
> runstate information:
>
> spin_lock include/linux/spinlock.h:354 [inline]
> get_kvmclock_ns+0x25/0x390 arch/x86/kvm/x86.c:2587
> kvm_xen_update_runstate+0x3d/0x2c0 arch/x86/kvm/xen.c:69
> kvm_xen_update_runstate_guest+0x74/0x320 arch/x86/kvm/xen.c:100
> kvm_xen_runstate_set_preempted arch/x86/kvm/xen.h:96 [inline]
> kvm_arch_vcpu_put+0x2d8/0x5a0 arch/x86/kvm/x86.c:4062
>
> So change the users of the spinlock to spin_lock_irqsave and
> spin_unlock_irqrestore.
>
> Reported-by: syzbot+b282b65c2c68492df769@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 30b5c851af79 ("KVM: x86/xen: Add support for vCPU runstate information")
> Cc: David Woodhouse <dwmw@xxxxxxxxxxxx>
> Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Reviewed-by: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> ---
> arch/x86/kvm/x86.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0a83eff40b43..2bfd00da465f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2329,7 +2329,7 @@ static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data)
> kvm_vcpu_write_tsc_offset(vcpu, offset);
> raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);
>
> - spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
> + spin_lock_irqsave(&kvm->arch.pvclock_gtod_sync_lock, flags);
> if (!matched) {
> kvm->arch.nr_vcpus_matched_tsc = 0;
> } else if (!already_matched) {
> @@ -2337,7 +2337,7 @@ static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data)
> }
>
> kvm_track_tsc_matching(vcpu);
> - spin_unlock(&kvm->arch.pvclock_gtod_sync_lock);
> + spin_unlock_irqrestore(&kvm->arch.pvclock_gtod_sync_lock, flags);
> }
>
> static inline void adjust_tsc_offset_guest(struct kvm_vcpu *vcpu,
> @@ -2559,15 +2559,16 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
> int i;
> struct kvm_vcpu *vcpu;
> struct kvm_arch *ka = &kvm->arch;
> + unsigned long flags;
>
> kvm_hv_invalidate_tsc_page(kvm);
>
> kvm_make_mclock_inprogress_request(kvm);
>
> /* no guest entries from this point */
> - spin_lock(&ka->pvclock_gtod_sync_lock);
> + spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
> pvclock_update_vm_gtod_copy(kvm);
> - spin_unlock(&ka->pvclock_gtod_sync_lock);
> + spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
>
> kvm_for_each_vcpu(i, vcpu, kvm)
> kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> @@ -2582,17 +2583,18 @@ u64 get_kvmclock_ns(struct kvm *kvm)
> {
> struct kvm_arch *ka = &kvm->arch;
> struct pvclock_vcpu_time_info hv_clock;
> + unsigned long flags;
> u64 ret;
>
> - spin_lock(&ka->pvclock_gtod_sync_lock);
> + spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
> if (!ka->use_master_clock) {
> - spin_unlock(&ka->pvclock_gtod_sync_lock);
> + spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
> return get_kvmclock_base_ns() + ka->kvmclock_offset;
> }
>
> hv_clock.tsc_timestamp = ka->master_cycle_now;
> hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
> - spin_unlock(&ka->pvclock_gtod_sync_lock);
> + spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
>
> /* both __this_cpu_read() and rdtsc() should be on the same cpu */
> get_cpu();
> @@ -2686,13 +2688,13 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
> * If the host uses TSC clock, then passthrough TSC as stable
> * to the guest.
> */
> - spin_lock(&ka->pvclock_gtod_sync_lock);
> + spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
> use_master_clock = ka->use_master_clock;
> if (use_master_clock) {
> host_tsc = ka->master_cycle_now;
> kernel_ns = ka->master_kernel_ns;
> }
> - spin_unlock(&ka->pvclock_gtod_sync_lock);
> + spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
>
> /* Keep irq disabled to prevent changes to the clock */
> local_irq_save(flags);
> @@ -7724,6 +7726,7 @@ static void kvm_hyperv_tsc_notifier(void)
> struct kvm *kvm;
> struct kvm_vcpu *vcpu;
> int cpu;
> + unsigned long flags;
>
> mutex_lock(&kvm_lock);
> list_for_each_entry(kvm, &vm_list, vm_list)
> @@ -7739,9 +7742,9 @@ static void kvm_hyperv_tsc_notifier(void)
> list_for_each_entry(kvm, &vm_list, vm_list) {
> struct kvm_arch *ka = &kvm->arch;
>
> - spin_lock(&ka->pvclock_gtod_sync_lock);
> + spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
> pvclock_update_vm_gtod_copy(kvm);
> - spin_unlock(&ka->pvclock_gtod_sync_lock);
> + spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
>
> kvm_for_each_vcpu(cpu, vcpu, kvm)
> kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> --
> 2.26.2
>
