Re: [PATCH 2/2] KVM: x86: disable interrupts while pvclock_gtod_sync_lock is taken

[Wanpeng Li]

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
>