| Message ID | 20170122090453.88101-1-dvyukov@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 22.01.2017 um 10:04 schrieb Dmitry Vyukov: > Currently svm_vm_data_hash_lock is left uninitialized. > This causes lockdep warnings. Properly initialize it. > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Joerg Roedel <joro@8bytes.org> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > Cc: kvm@vger.kernel.org > Cc: syzkaller@googlegroups.com > --- > arch/x86/kvm/svm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 08a4d3ab3455..b928a9c34987 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -972,7 +972,7 @@ static void svm_disable_lbrv(struct vcpu_svm *svm) > */ > #define SVM_VM_DATA_HASH_BITS 8 > DECLARE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS); > -static spinlock_t svm_vm_data_hash_lock; > +static DEFINE_SPINLOCK(svm_vm_data_hash_lock); > > /* Note: > * This function is called from IOMMU driver to notify > We have spin_lock_init(&svm_vm_data_hash_lock); in svm_hardware_setup(). If this isn't called, wouldn't the right fix be to find out why?
On Mon, Jan 23, 2017 at 3:25 PM, David Hildenbrand <david@redhat.com> wrote: > Am 22.01.2017 um 10:04 schrieb Dmitry Vyukov: >> Currently svm_vm_data_hash_lock is left uninitialized. >> This causes lockdep warnings. Properly initialize it. >> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com> >> Cc: Joerg Roedel <joro@8bytes.org> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com> >> Cc: kvm@vger.kernel.org >> Cc: syzkaller@googlegroups.com >> --- >> arch/x86/kvm/svm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 08a4d3ab3455..b928a9c34987 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -972,7 +972,7 @@ static void svm_disable_lbrv(struct vcpu_svm *svm) >> */ >> #define SVM_VM_DATA_HASH_BITS 8 >> DECLARE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS); >> -static spinlock_t svm_vm_data_hash_lock; >> +static DEFINE_SPINLOCK(svm_vm_data_hash_lock); >> >> /* Note: >> * This function is called from IOMMU driver to notify >> > > We have > > spin_lock_init(&svm_vm_data_hash_lock); > > in svm_hardware_setup(). > > If this isn't called, wouldn't the right fix be to find out why? spin_lock_init is called conditionally if avic. Then avic_vm_init returns 0, if !avic. But then avic_vm_destroy does not check avic and unconditionally uses the spinlock. Perhaps the right fix then will be: @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) unsigned long flags; struct kvm_arch *vm_data = &kvm->arch; + if (!avic) + return 0; + avic_free_vm_id(vm_data->avic_vm_id); if (vm_data->avic_logical_id_table_page) Unfortunately I don't remember how I managed to trigger this warning because I don't have any SVM-capable hardware... But I remember that I did not do anything special besides just enabling spinlock checks and then doing something trivial. Could somebody try to use SVM with the spinlock checks enabled? I don't feel comfortable sending a non-trivial patch without testing it...
Am 23.01.2017 um 15:52 schrieb Dmitry Vyukov: > On Mon, Jan 23, 2017 at 3:25 PM, David Hildenbrand <david@redhat.com> wrote: >> Am 22.01.2017 um 10:04 schrieb Dmitry Vyukov: >>> Currently svm_vm_data_hash_lock is left uninitialized. >>> This causes lockdep warnings. Properly initialize it. >>> >>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com> >>> Cc: Joerg Roedel <joro@8bytes.org> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: "Radim Krčmář" <rkrcmar@redhat.com> >>> Cc: kvm@vger.kernel.org >>> Cc: syzkaller@googlegroups.com >>> --- >>> arch/x86/kvm/svm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >>> index 08a4d3ab3455..b928a9c34987 100644 >>> --- a/arch/x86/kvm/svm.c >>> +++ b/arch/x86/kvm/svm.c >>> @@ -972,7 +972,7 @@ static void svm_disable_lbrv(struct vcpu_svm *svm) >>> */ >>> #define SVM_VM_DATA_HASH_BITS 8 >>> DECLARE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS); >>> -static spinlock_t svm_vm_data_hash_lock; >>> +static DEFINE_SPINLOCK(svm_vm_data_hash_lock); >>> >>> /* Note: >>> * This function is called from IOMMU driver to notify >>> >> >> We have >> >> spin_lock_init(&svm_vm_data_hash_lock); >> >> in svm_hardware_setup(). >> >> If this isn't called, wouldn't the right fix be to find out why? > > > spin_lock_init is called conditionally if avic. > Then avic_vm_init returns 0, if !avic. > But then avic_vm_destroy does not check avic and unconditionally uses > the spinlock. > Perhaps the right fix then will be: > > @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) > unsigned long flags; > struct kvm_arch *vm_data = &kvm->arch; > > + if (!avic) > + return 0; > + > avic_free_vm_id(vm_data->avic_vm_id); > > if (vm_data->avic_logical_id_table_page) > > > Unfortunately I don't remember how I managed to trigger this warning > because I don't have any SVM-capable hardware... > But I remember that I did not do anything special besides just > enabling spinlock checks and then doing something trivial. > Could somebody try to use SVM with the spinlock checks enabled? I > don't feel comfortable sending a non-trivial patch without testing > it... > Or stick to your initial patch and simply remove the previous initialization (sounds clean to me). But we'd better double check if that additional check in avic_vm_destroy() is also reasonable (maybe something else is touched that shouldn't be).
On 23/01/2017 15:52, Dmitry Vyukov wrote: > > @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) > unsigned long flags; > struct kvm_arch *vm_data = &kvm->arch; > > + if (!avic) > + return 0; > + > avic_free_vm_id(vm_data->avic_vm_id); > > if (vm_data->avic_logical_id_table_page) Yes, this is the right one. Paolo
On Tue, Jan 24, 2017 at 12:32 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 23/01/2017 15:52, Dmitry Vyukov wrote: >> >> @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) >> unsigned long flags; >> struct kvm_arch *vm_data = &kvm->arch; >> >> + if (!avic) >> + return 0; >> + >> avic_free_vm_id(vm_data->avic_vm_id); >> >> if (vm_data->avic_logical_id_table_page) > > Yes, this is the right one. I can mail this patch, but I can only test build.
On 23/01/2017 17:09, David Hildenbrand wrote: >> @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) >> unsigned long flags; >> struct kvm_arch *vm_data = &kvm->arch; >> >> + if (!avic) >> + return 0; >> + >> avic_free_vm_id(vm_data->avic_vm_id); >> >> if (vm_data->avic_logical_id_table_page) >> >> >> Unfortunately I don't remember how I managed to trigger this warning >> because I don't have any SVM-capable hardware... >> But I remember that I did not do anything special besides just >> enabling spinlock checks and then doing something trivial. >> Could somebody try to use SVM with the spinlock checks enabled? I >> don't feel comfortable sending a non-trivial patch without testing >> it... >> > Or stick to your initial patch and simply remove the previous > initialization (sounds clean to me). But we'd better double check if > that additional check in avic_vm_destroy() is also reasonable (maybe > something else is touched that shouldn't be). Yes, that second patch is the right one. David, if you wish to submit a patch to simplify the initialization, I'll apply that too. Thanks, Paolo
On 24/01/2017 12:34, Dmitry Vyukov wrote: > On Tue, Jan 24, 2017 at 12:32 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 23/01/2017 15:52, Dmitry Vyukov wrote: >>> >>> @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) >>> unsigned long flags; >>> struct kvm_arch *vm_data = &kvm->arch; >>> >>> + if (!avic) >>> + return 0; >>> + >>> avic_free_vm_id(vm_data->avic_vm_id); >>> >>> if (vm_data->avic_logical_id_table_page) >> >> Yes, this is the right one. > > > I can mail this patch, but I can only test build. No problem, I have applied it locally and I am testing it. Paolo
Mailed a new patch "kvm: fix usage of uninit spinlock in avic_vm_destroy()". On Tue, Jan 24, 2017 at 12:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 24/01/2017 12:34, Dmitry Vyukov wrote: >> On Tue, Jan 24, 2017 at 12:32 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 23/01/2017 15:52, Dmitry Vyukov wrote: >>>> >>>> @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) >>>> unsigned long flags; >>>> struct kvm_arch *vm_data = &kvm->arch; >>>> >>>> + if (!avic) >>>> + return 0; >>>> + >>>> avic_free_vm_id(vm_data->avic_vm_id); >>>> >>>> if (vm_data->avic_logical_id_table_page) >>> >>> Yes, this is the right one. >> >> >> I can mail this patch, but I can only test build. > > No problem, I have applied it locally and I am testing it. > > Paolo
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 08a4d3ab3455..b928a9c34987 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -972,7 +972,7 @@ static void svm_disable_lbrv(struct vcpu_svm *svm) */ #define SVM_VM_DATA_HASH_BITS 8 DECLARE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS); -static spinlock_t svm_vm_data_hash_lock; +static DEFINE_SPINLOCK(svm_vm_data_hash_lock); /* Note: * This function is called from IOMMU driver to notify
Currently svm_vm_data_hash_lock is left uninitialized. This causes lockdep warnings. Properly initialize it. Signed-off-by: Dmitry Vyukov <dvyukov@google.com> Cc: Joerg Roedel <joro@8bytes.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: kvm@vger.kernel.org Cc: syzkaller@googlegroups.com --- arch/x86/kvm/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)