-----Original Message----- From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo Bonzini Sent: Friday, October 21, 2016 1:07 AM To: Xiao Guangrong <guangrong.x...@linux.intel.com>; Xiao, Guangrong <guangrong.x...@intel.com>; Song, Jike <jike.s...@intel.com>; Alex Williamson <alex.william...@redhat.com> Cc: Tian, Kevin <kevin.t...@intel.com>; Neo Jia <c...@nvidia.com>; k...@vger.kernel.org; qemu-devel <qemu-devel@nongnu.org>; Chen, Xiaoguang <xiaoguang.c...@intel.com>; Kirti Wankhede <kwankh...@nvidia.com> Subject: Re: [Qemu-devel] [PATCH 1/2] KVM: page track: add a new notifier type: track_flush_slot
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. Yes, mutex can fix it and is good to me. :)