[PATCH] kvm: initialize SVM spinlock

[PATCH] kvm: initialize SVM spinlock

[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
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH] kvm: initialize SVM spinlock

[David Hildenbrand]

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?

-- 

David

Re: [PATCH] kvm: initialize SVM spinlock

[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...

Re: [PATCH] kvm: initialize SVM spinlock

[David Hildenbrand]

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).

-- 

David

Re: [PATCH] kvm: initialize SVM spinlock

[Paolo Bonzini]

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

Re: [PATCH] kvm: initialize SVM spinlock

[Dmitry Vyukov]

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.

Re: [PATCH] kvm: initialize SVM spinlock

[Paolo Bonzini]

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

Re: [PATCH] kvm: initialize SVM spinlock

[Paolo Bonzini]

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

Re: [PATCH] kvm: initialize SVM spinlock

[Dmitry Vyukov]

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