Re: [Qemu-devel] [PATCH 1/2] KVM: page track: add a new notifier type: track_flush_slot

[Jike Song]

On 10/26/2016 10:45 PM, Paolo Bonzini wrote:
> On 26/10/2016 15:44, Jike Song wrote:
>> On 10/21/2016 01:06 AM, Paolo Bonzini wrote:
>>> On 20/10/2016 03:48, Xiao Guangrong wrote:
>>>> I understood that KVM side is safe, however, vfio side is independent with
>>>> kvm and the user of usrdata can fetch kvm struct at any time, consider
>>>> this scenario:
>>>>
>>>> CPU 0                         CPU 1
>>>> KVM:                         VFIO/userdata user
>>>>   kvm_ioctl_create_device
>>>>      get_kvm()
>>>>                             vfio_group_get_usrdata(vfio_group)
>>>>   kvm_device_release
>>>>     put_kvm()
>>>>                             !!! kvm refcount has gone
>>>>                             use KVM struct
>>>>
>>>> Then, the user of userdata have fetched kvm struct but the refcount has
>>>> already gone.
>>>
>>> vfio_group_set_usrdata (actually) kvm_vfio_group_set_kvm has called
>>> kvm_get_kvm too, however.  What you need is a mutex that is taken by
>>> vfio_group_set_usrdata and by the callers of vfio_group_get_usrdata.
>>
>> Hi Paolo & Guangrong,
>>
>> I walked the whole thread and became a little nervous: I don't want
>> to introduce a global mutex.
>>
>> The problem is, as I understand, vfio_group_get_usrdata() returns a
>> KVM pointer but it may be stale. To make the pointer always valid,
>> it can call kvm_get_kvm() *before* return the pointer.
> 
> That doesn't work, you still have to protect get against concurrent set.
>  But the mutex need not be global, it is specific to the vfio device.
> You probably have such a mutex anyway...

Thanks Paolo, I agree whatsoever a mutex is necessary. I cooked a patch
sent to you and Alex, please kindly have a look :-)

--
Thanks,
Jike

>> I would apologize in advance if this idea turns out totally
>> nonsense, but hey, please kindly help fix my whim :-)
>>
>>
>> [vfio.h]
>>
>>      struct vfio_usrdata {
>>              void *data;
>>              void (*get)(void *data);
>>              void (*put)(void *data)
>>      };
>>
>>      vfio_group {
>>              ...
>>              vfio_usrdata *usrdata;
>>
>> [kvm.ko]
>>
>>      struvt vfio_usrdata kvmdata = {
>>              .data = kvm,
>>              .get = kvm_get_kvm,
>>              .put = kvm_put_kvm,
>>      };
>>
>>      fn = symbol_get(vfio_group_set_usrdata)
>>      fn(vfio_group, &kvmdata)
>>
>>
>> [vfio.ko]
>>
>>      vfio_group_set_usrdata
>>              lock
>>              vfio_group->d = kvmdata
>>              unlock
>>
>>      void *vfio_group_get_usrdata
>>              lock
>>              struct vfio_usrdata *d = vfio_group->usrdata;
>>              d->get(d->data);
>>              unlock
>>              return d->data;
>>
>>      void vfio_group_put_usrdata
>>              lock
>>              struct vfio_usrdata *d = vfio_group->usrdata;
>>              d->put(d->data)
>>              unlock
>>
>> [kvmgt.ko]
>>
>>      call vfio_group_get_usrdata to get kvm,
>>      call vfio_group_put_usrdata to release it
>>      *never* call kvm_get_kvm/kvm_put_kvm