Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/08/2016 16:49, Christoffer Dall wrote:
> On Tue, Aug 09, 2016 at 03:16:26PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 09/08/2016 14:55, Christoffer Dall wrote:
>>> On Tue, Aug 09, 2016 at 02:37:43PM +0200, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 09/08/2016 14:20, Christoffer Dall wrote:
>>>>> KVM devices were manipulating list data structures without any form of
>>>>> synchronization, and some implementations of the create operations also
>>>>> suffered from a lack of synchronization.
>>>>>
>>>>> Now when we've split the xics create operation into create and init, we
>>>>> can hold the kvm->lock mutex while calling the create operation and when
>>>>> manipulating the devices list.
>>>>>
>>>>> The error path in the generic code gets slightly ugly because we have to
>>>>> take the mutex again and delete the device from the list, but holding
>>>>> the mutex during anon_inode_getfd or releasing/locking the mutex in the
>>>>> common non-error path seemed wrong.
>>>>>
>>>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>>
>>>> Very nice (and small), but please add a comment to the create member in
>>>> kvm_device_ops.
>>>
>>> Like this?:
>>>
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> index d3c9b82..9c28b4d 100644
>>> --- a/include/linux/kvm_host.h
>>> +++ b/include/linux/kvm_host.h
>>> @@ -1113,6 +1113,12 @@ struct kvm_device {
>>>  /* create, destroy, and name are mandatory */
>>>  struct kvm_device_ops {
>>>  	const char *name;
>>> +
>>> +	/*
>>> +	 * create is called holding kvm->lock and any operations not suitable
>>> +	 * to do while holding the lock should be deferred to init (see
>>> +	 * below).
>>> +	 */
>>>  	int (*create)(struct kvm_device *dev, u32 type);
>>>  
>>>  	/*
>>>
>>
>> That's okay, series
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks, I'll send a v2.  Will you just apply the patches to kvm/master
> or would you like me to include it in my pull request for -rc2 ?

The tree is currently in Radim's hands, but I expect this to be applied
directly by one of us.

> Also, do you want to wait for a tested-by from the other arch
> maintainers?

That would be nice, but not mandatory.

Paolo