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

[Paolo Bonzini]

On 20/10/2016 03:48, Xiao Guangrong wrote:
> 
> 
> On 10/19/2016 10:14 PM, Paolo Bonzini wrote:
>>
>>
>> On 19/10/2016 15:39, Xiao Guangrong wrote:
>>>
>>>
>>> On 10/19/2016 07:56 PM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 19/10/2016 07:45, Xiao Guangrong wrote:
>>>>>
>>>>>
>>>>> On 10/19/2016 10:32 AM, Jike Song wrote:
>>>>> +EXPORT_SYMBOL_GPL(vfio_group_set_usrdata);
>>>>>>>> +
>>>>>>>> +void *vfio_group_get_usrdata(struct vfio_group *group)
>>>>>>>> +{
>>>>>>>> +    return group->usrdata;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL_GPL(vfio_group_get_usrdata);
>>>>>>>> +
>>>>>>>> +void *vfio_group_get_usrdata_by_device(struct device *dev)
>>>>>>>> +{
>>>>>>>> +    struct vfio_group *vfio_group;
>>>>>>>> +
>>>>>>>> +    vfio_group = __vfio_group_get_from_iommu(dev->iommu_group);
>>>>>>>
>>>>>>> We actually need to use iommu_group_get() here.  Kirti adds a
>>>>>>> vfio_group_get_from_dev() in v9 03/12 that does this properly.
>>>>>>>
>>>>>>>> +    if (!vfio_group)
>>>>>>>> +        return NULL;
>>>>>>>> +
>>>>>>>> +    return vfio_group_get_usrdata(vfio_group);
>>>>>
>>>>> I am worrying if the kvm instance got from group->usrdata is safe
>>>>> enough? What happens if you get the instance after kvm released
>>>>> kvm-vfio device?
>>>>
>>>> It shouldn't happen if you use kvm_get_kvm and kvm_put_kvm
>>>> properly.  It
>>>> is almost okay in the patch, just:
>>>
>>> How about if KVM releases kvm-vfio device between
>>> vfio_group_get_usrdata()
>>> and get_kvm()?
>>
>> That cannot happen as long as there is a struct file* for the device
>> (see kvm_ioctl_create_device and kvm_device_release).  Since you're
>> sending a ioctl to it, it's fine.
> 
> 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.

Paolo

> What i missed?
> -- 
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>