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

[Paolo Bonzini <pbonzini@redhat.com>]

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

Paolo

> 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
> 
> --
> Thanks,
> Jike
>