Message ID | 20230202162442.78216-1-mjrosato@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] vfio: fix deadlock between group lock and kvm lock | expand |
On Thu, 2 Feb 2023 11:24:42 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > After 51cdc8bc120e, we have another deadlock scenario between the > kvm->lock and the vfio group_lock with two different codepaths acquiring > the locks in different order. Specifically in vfio_open_device, vfio > holds the vfio group_lock when issuing device->ops->open_device but some > drivers (like vfio-ap) need to acquire kvm->lock during their open_device > routine; Meanwhile, kvm_vfio_release will acquire the kvm->lock first > before calling vfio_file_set_kvm which will acquire the vfio group_lock. > > To resolve this, let's remove the need for the vfio group_lock from the > kvm_vfio_release codepath. This is done by introducing a new spinlock to > protect modifications to the vfio group kvm pointer, and acquiring a kvm > ref from within vfio while holding this spinlock, with the reference held > until the last close for the device in question. > > Fixes: 51cdc8bc120e ("kvm/vfio: Fix potential deadlock on vfio group_lock") > Reported-by: Anthony Krowiak <akrowiak@linux.ibm.com> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > Changes from v2: > * Relocate the new functions back to vfio_main and externalize to call > from group (Kevin) since cdev will need this too > * s/vfio_kvm_*_kvm/vfio_device_*_kvm/ and only pass device as input. > Handle new kvm_ref_lock directly inside vfio_device_get_kvm (Alex) > * Add assert_lockdep_held for dev_set lock (Alex) > * Internalize error paths for vfio_device_get_kvm_safe and now return > void - either device->kvm is set with a ref taken or is NULL (Alex) > * Other flow suggestions to make the call path cleaner - Thanks! (Alex) > * Can't pass group->kvm to vfio_device_open, as it references the value > outside of new lock. Pass device->kvm to minimize changes in this > fix (Alex, Yi) > Changes from v1: > * use spin_lock instead of spin_lock_irqsave (Jason) > * clear device->kvm_put as part of vfio_kvm_put_kvm (Yi) > * Re-arrange code to avoid referencing the group contents from within > vfio_main (Kevin) which meant moving most of the code in this patch > to group.c along with getting/dropping of the dev_set lock > --- > drivers/vfio/group.c | 32 ++++++++++++++---- > drivers/vfio/vfio.h | 14 ++++++++ > drivers/vfio/vfio_main.c | 70 ++++++++++++++++++++++++++++++++++++---- > include/linux/vfio.h | 2 +- > 4 files changed, 103 insertions(+), 15 deletions(-) LGTM. I'm not sure moving the functions to vfio_main really buys us anything since we're making so much use of group fields. The cdev approach will necessarily be different, so the bulk of the get code will likely need to move back to group.c anyway. I'll wait for further comments and reviews by others before applying. Thanks, Alex > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index bb24b2f0271e..7fed4233ca23 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -164,13 +164,23 @@ static int vfio_device_group_open(struct vfio_device *device) > goto out_unlock; > } > > + mutex_lock(&device->dev_set->lock); > + > /* > - * Here we pass the KVM pointer with the group under the lock. If the > - * device driver will use it, it must obtain a reference and release it > - * during close_device. > + * Before the first device open, get the KVM pointer currently > + * associated with the group (if there is one) and obtain a reference > + * now that will be held until the open_count reaches 0 again. Save > + * the pointer in the device for use by drivers. > */ > - ret = vfio_device_open(device, device->group->iommufd, > - device->group->kvm); > + if (device->open_count == 0) > + vfio_device_get_kvm_safe(device); > + > + ret = vfio_device_open(device, device->group->iommufd, device->kvm); > + > + if (device->open_count == 0) > + vfio_device_put_kvm(device); > + > + mutex_unlock(&device->dev_set->lock); > > out_unlock: > mutex_unlock(&device->group->group_lock); > @@ -180,7 +190,14 @@ static int vfio_device_group_open(struct vfio_device *device) > void vfio_device_group_close(struct vfio_device *device) > { > mutex_lock(&device->group->group_lock); > + mutex_lock(&device->dev_set->lock); > + > vfio_device_close(device, device->group->iommufd); > + > + if (device->open_count == 0) > + vfio_device_put_kvm(device); > + > + mutex_unlock(&device->dev_set->lock); > mutex_unlock(&device->group->group_lock); > } > > @@ -450,6 +467,7 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group, > > refcount_set(&group->drivers, 1); > mutex_init(&group->group_lock); > + spin_lock_init(&group->kvm_ref_lock); > INIT_LIST_HEAD(&group->device_list); > mutex_init(&group->device_lock); > group->iommu_group = iommu_group; > @@ -803,9 +821,9 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm) > if (!vfio_file_is_group(file)) > return; > > - mutex_lock(&group->group_lock); > + spin_lock(&group->kvm_ref_lock); > group->kvm = kvm; > - mutex_unlock(&group->group_lock); > + spin_unlock(&group->kvm_ref_lock); > } > EXPORT_SYMBOL_GPL(vfio_file_set_kvm); > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index f8219a438bfb..20d715b0a3a8 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -74,6 +74,7 @@ struct vfio_group { > struct file *opened_file; > struct blocking_notifier_head notifier; > struct iommufd_ctx *iommufd; > + spinlock_t kvm_ref_lock; > }; > > int vfio_device_set_group(struct vfio_device *device, > @@ -251,4 +252,17 @@ extern bool vfio_noiommu __read_mostly; > enum { vfio_noiommu = false }; > #endif > > +#ifdef CONFIG_HAVE_KVM > +void vfio_device_get_kvm_safe(struct vfio_device *device); > +void vfio_device_put_kvm(struct vfio_device *device); > +#else > +static inline void vfio_device_get_kvm_safe(struct vfio_device *device) > +{ > +} > + > +static inline void vfio_device_put_kvm(struct vfio_device *device) > +{ > +} > +#endif > + > #endif > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 5177bb061b17..4762550e9f42 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -16,6 +16,9 @@ > #include <linux/fs.h> > #include <linux/idr.h> > #include <linux/iommu.h> > +#ifdef CONFIG_HAVE_KVM > +#include <linux/kvm_host.h> > +#endif > #include <linux/list.h> > #include <linux/miscdevice.h> > #include <linux/module.h> > @@ -338,6 +341,62 @@ void vfio_unregister_group_dev(struct vfio_device *device) > } > EXPORT_SYMBOL_GPL(vfio_unregister_group_dev); > > +#ifdef CONFIG_HAVE_KVM > +void vfio_device_get_kvm_safe(struct vfio_device *device) > +{ > + void (*pfn)(struct kvm *kvm); > + bool (*fn)(struct kvm *kvm); > + bool ret; > + > + lockdep_assert_held(&device->dev_set->lock); > + > + spin_lock(&device->group->kvm_ref_lock); > + if (!device->group->kvm) > + goto unlock; > + > + pfn = symbol_get(kvm_put_kvm); > + if (WARN_ON(!pfn)) > + goto unlock; > + > + fn = symbol_get(kvm_get_kvm_safe); > + if (WARN_ON(!fn)) { > + symbol_put(kvm_put_kvm); > + goto unlock; > + } > + > + ret = fn(device->group->kvm); > + symbol_put(kvm_get_kvm_safe); > + if (!ret) { > + symbol_put(kvm_put_kvm); > + goto unlock; > + } > + > + device->put_kvm = pfn; > + device->kvm = device->group->kvm; > + > +unlock: > + spin_unlock(&device->group->kvm_ref_lock); > +} > + > +void vfio_device_put_kvm(struct vfio_device *device) > +{ > + lockdep_assert_held(&device->dev_set->lock); > + > + if (!device->kvm) > + return; > + > + if (WARN_ON(!device->put_kvm)) > + goto clear; > + > + device->put_kvm(device->kvm); > + device->put_kvm = NULL; > + symbol_put(kvm_put_kvm); > + > +clear: > + device->kvm = NULL; > +} > +#endif > + > /* true if the vfio_device has open_device() called but not close_device() */ > static bool vfio_assert_device_open(struct vfio_device *device) > { > @@ -361,7 +420,6 @@ static int vfio_device_first_open(struct vfio_device *device, > if (ret) > goto err_module_put; > > - device->kvm = kvm; > if (device->ops->open_device) { > ret = device->ops->open_device(device); > if (ret) > @@ -370,7 +428,6 @@ static int vfio_device_first_open(struct vfio_device *device, > return 0; > > err_unuse_iommu: > - device->kvm = NULL; > if (iommufd) > vfio_iommufd_unbind(device); > else > @@ -387,7 +444,6 @@ static void vfio_device_last_close(struct vfio_device *device, > > if (device->ops->close_device) > device->ops->close_device(device); > - device->kvm = NULL; > if (iommufd) > vfio_iommufd_unbind(device); > else > @@ -400,14 +456,14 @@ int vfio_device_open(struct vfio_device *device, > { > int ret = 0; > > - mutex_lock(&device->dev_set->lock); > + lockdep_assert_held(&device->dev_set->lock); > + > device->open_count++; > if (device->open_count == 1) { > ret = vfio_device_first_open(device, iommufd, kvm); > if (ret) > device->open_count--; > } > - mutex_unlock(&device->dev_set->lock); > > return ret; > } > @@ -415,12 +471,12 @@ int vfio_device_open(struct vfio_device *device, > void vfio_device_close(struct vfio_device *device, > struct iommufd_ctx *iommufd) > { > - mutex_lock(&device->dev_set->lock); > + lockdep_assert_held(&device->dev_set->lock); > + > vfio_assert_device_open(device); > if (device->open_count == 1) > vfio_device_last_close(device, iommufd); > device->open_count--; > - mutex_unlock(&device->dev_set->lock); > } > > /* > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 35be78e9ae57..87ff862ff555 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -46,7 +46,6 @@ struct vfio_device { > struct vfio_device_set *dev_set; > struct list_head dev_set_list; > unsigned int migration_flags; > - /* Driver must reference the kvm during open_device or never touch it */ > struct kvm *kvm; > > /* Members below here are private, not for driver use */ > @@ -58,6 +57,7 @@ struct vfio_device { > struct list_head group_next; > struct list_head iommu_entry; > struct iommufd_access *iommufd_access; > + void (*put_kvm)(struct kvm *kvm); > #if IS_ENABLED(CONFIG_IOMMUFD) > struct iommufd_device *iommufd_device; > struct iommufd_ctx *iommufd_ictx;
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Friday, February 3, 2023 3:42 AM > > On Thu, 2 Feb 2023 11:24:42 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > After 51cdc8bc120e, we have another deadlock scenario between the > > kvm->lock and the vfio group_lock with two different codepaths acquiring > > the locks in different order. Specifically in vfio_open_device, vfio > > holds the vfio group_lock when issuing device->ops->open_device but > some > > drivers (like vfio-ap) need to acquire kvm->lock during their open_device > > routine; Meanwhile, kvm_vfio_release will acquire the kvm->lock first > > before calling vfio_file_set_kvm which will acquire the vfio group_lock. > > > > To resolve this, let's remove the need for the vfio group_lock from the > > kvm_vfio_release codepath. This is done by introducing a new spinlock to > > protect modifications to the vfio group kvm pointer, and acquiring a kvm > > ref from within vfio while holding this spinlock, with the reference held > > until the last close for the device in question. > > > > Fixes: 51cdc8bc120e ("kvm/vfio: Fix potential deadlock on vfio group_lock") > > Reported-by: Anthony Krowiak <akrowiak@linux.ibm.com> > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > > --- > > Changes from v2: > > * Relocate the new functions back to vfio_main and externalize to call > > from group (Kevin) since cdev will need this too > > * s/vfio_kvm_*_kvm/vfio_device_*_kvm/ and only pass device as input. > > Handle new kvm_ref_lock directly inside vfio_device_get_kvm (Alex) > > * Add assert_lockdep_held for dev_set lock (Alex) > > * Internalize error paths for vfio_device_get_kvm_safe and now return > > void - either device->kvm is set with a ref taken or is NULL (Alex) > > * Other flow suggestions to make the call path cleaner - Thanks! (Alex) > > * Can't pass group->kvm to vfio_device_open, as it references the value > > outside of new lock. Pass device->kvm to minimize changes in this > > fix (Alex, Yi) > > Changes from v1: > > * use spin_lock instead of spin_lock_irqsave (Jason) > > * clear device->kvm_put as part of vfio_kvm_put_kvm (Yi) > > * Re-arrange code to avoid referencing the group contents from within > > vfio_main (Kevin) which meant moving most of the code in this patch > > to group.c along with getting/dropping of the dev_set lock > > --- > > drivers/vfio/group.c | 32 ++++++++++++++---- > > drivers/vfio/vfio.h | 14 ++++++++ > > drivers/vfio/vfio_main.c | 70 ++++++++++++++++++++++++++++++++++++-- > -- > > include/linux/vfio.h | 2 +- > > 4 files changed, 103 insertions(+), 15 deletions(-) > > LGTM. I'm not sure moving the functions to vfio_main really buys us > anything since we're making so much use of group fields. The cdev > approach will necessarily be different, so the bulk of the get code will > likely need to move back to group.c anyway. > well my last comment was based on Matthew's v2 where the get code gets a kvm passed in instead of implicitly retrieving group ref_lock internally. In that case the get/put helpers only contain device logic thus fit in vfio_main.c. with v3 then they have to be in group.c since we don't want to use group fields in vfio_main.c. but I still think v2 of the helpers is slightly better. The only difference between cdev and group when handling this race is using different ref_lock. the symbol get/put part is exactly same. So even if we merge v3 like this, very likely Yi has to change it back to v2 style to share the get/put helpers while just leaving the ref_lock part handled differently between the two path. Thanks Kevin
On Thu, 2 Feb 2023 23:04:10 +0000 "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Friday, February 3, 2023 3:42 AM > > > > On Thu, 2 Feb 2023 11:24:42 -0500 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > > > After 51cdc8bc120e, we have another deadlock scenario between the > > > kvm->lock and the vfio group_lock with two different codepaths acquiring > > > the locks in different order. Specifically in vfio_open_device, vfio > > > holds the vfio group_lock when issuing device->ops->open_device but > > some > > > drivers (like vfio-ap) need to acquire kvm->lock during their open_device > > > routine; Meanwhile, kvm_vfio_release will acquire the kvm->lock first > > > before calling vfio_file_set_kvm which will acquire the vfio group_lock. > > > > > > To resolve this, let's remove the need for the vfio group_lock from the > > > kvm_vfio_release codepath. This is done by introducing a new spinlock to > > > protect modifications to the vfio group kvm pointer, and acquiring a kvm > > > ref from within vfio while holding this spinlock, with the reference held > > > until the last close for the device in question. > > > > > > Fixes: 51cdc8bc120e ("kvm/vfio: Fix potential deadlock on vfio group_lock") > > > Reported-by: Anthony Krowiak <akrowiak@linux.ibm.com> > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > --- > > > Changes from v2: > > > * Relocate the new functions back to vfio_main and externalize to call > > > from group (Kevin) since cdev will need this too > > > * s/vfio_kvm_*_kvm/vfio_device_*_kvm/ and only pass device as input. > > > Handle new kvm_ref_lock directly inside vfio_device_get_kvm (Alex) > > > * Add assert_lockdep_held for dev_set lock (Alex) > > > * Internalize error paths for vfio_device_get_kvm_safe and now return > > > void - either device->kvm is set with a ref taken or is NULL (Alex) > > > * Other flow suggestions to make the call path cleaner - Thanks! (Alex) > > > * Can't pass group->kvm to vfio_device_open, as it references the value > > > outside of new lock. Pass device->kvm to minimize changes in this > > > fix (Alex, Yi) > > > Changes from v1: > > > * use spin_lock instead of spin_lock_irqsave (Jason) > > > * clear device->kvm_put as part of vfio_kvm_put_kvm (Yi) > > > * Re-arrange code to avoid referencing the group contents from within > > > vfio_main (Kevin) which meant moving most of the code in this patch > > > to group.c along with getting/dropping of the dev_set lock > > > --- > > > drivers/vfio/group.c | 32 ++++++++++++++---- > > > drivers/vfio/vfio.h | 14 ++++++++ > > > drivers/vfio/vfio_main.c | 70 ++++++++++++++++++++++++++++++++++++-- > > -- > > > include/linux/vfio.h | 2 +- > > > 4 files changed, 103 insertions(+), 15 deletions(-) > > > > LGTM. I'm not sure moving the functions to vfio_main really buys us > > anything since we're making so much use of group fields. The cdev > > approach will necessarily be different, so the bulk of the get code will > > likely need to move back to group.c anyway. > > > > well my last comment was based on Matthew's v2 where the get code > gets a kvm passed in instead of implicitly retrieving group ref_lock > internally. In that case the get/put helpers only contain device logic > thus fit in vfio_main.c. > > with v3 then they have to be in group.c since we don't want to use > group fields in vfio_main.c. > > but I still think v2 of the helpers is slightly better. The only difference > between cdev and group when handling this race is using different > ref_lock. the symbol get/put part is exactly same. So even if we > merge v3 like this, very likely Yi has to change it back to v2 style > to share the get/put helpers while just leaving the ref_lock part > handled differently between the two path. I'm not really a fan of the asymmetry of the v2 version where the get helper needs to be called under the new kvm_ref_lock, but the put helper does not. Having the get helper handle that makes the caller much cleaner. Thanks, Alex
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Friday, February 3, 2023 7:13 AM > > On Thu, 2 Feb 2023 23:04:10 +0000 > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > Sent: Friday, February 3, 2023 3:42 AM > > > > > > > > > LGTM. I'm not sure moving the functions to vfio_main really buys us > > > anything since we're making so much use of group fields. The cdev > > > approach will necessarily be different, so the bulk of the get code will > > > likely need to move back to group.c anyway. > > > > > > > well my last comment was based on Matthew's v2 where the get code > > gets a kvm passed in instead of implicitly retrieving group ref_lock > > internally. In that case the get/put helpers only contain device logic > > thus fit in vfio_main.c. > > > > with v3 then they have to be in group.c since we don't want to use > > group fields in vfio_main.c. > > > > but I still think v2 of the helpers is slightly better. The only difference > > between cdev and group when handling this race is using different > > ref_lock. the symbol get/put part is exactly same. So even if we > > merge v3 like this, very likely Yi has to change it back to v2 style > > to share the get/put helpers while just leaving the ref_lock part > > handled differently between the two path. > > I'm not really a fan of the asymmetry of the v2 version where the get > helper needs to be called under the new kvm_ref_lock, but the put > helper does not. Having the get helper handle that makes the caller > much cleaner. Thanks, > What about passing the lock pointer into the helper? it's still slightly asymmetry as the put helper doesn't carry the lock pointer but it could also be interpreted as if the pointer has been saved in the get then if it needs to be referenced by the put there is no need to pass it in again.
Hi Matthew, > From: Matthew Rosato <mjrosato@linux.ibm.com> > Sent: Friday, February 3, 2023 12:25 AM > > After 51cdc8bc120e, we have another deadlock scenario between the > kvm->lock and the vfio group_lock with two different codepaths acquiring > the locks in different order. Specifically in vfio_open_device, vfio > holds the vfio group_lock when issuing device->ops->open_device but > some > drivers (like vfio-ap) need to acquire kvm->lock during their open_device > routine; Meanwhile, kvm_vfio_release will acquire the kvm->lock first > before calling vfio_file_set_kvm which will acquire the vfio group_lock. > > To resolve this, let's remove the need for the vfio group_lock from the > kvm_vfio_release codepath. This is done by introducing a new spinlock to > protect modifications to the vfio group kvm pointer, and acquiring a kvm > ref from within vfio while holding this spinlock, with the reference held > until the last close for the device in question. > > Fixes: 51cdc8bc120e ("kvm/vfio: Fix potential deadlock on vfio group_lock") > Reported-by: Anthony Krowiak <akrowiak@linux.ibm.com> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > Changes from v2: > * Relocate the new functions back to vfio_main and externalize to call > from group (Kevin) since cdev will need this too > * s/vfio_kvm_*_kvm/vfio_device_*_kvm/ and only pass device as input. > Handle new kvm_ref_lock directly inside vfio_device_get_kvm (Alex) > * Add assert_lockdep_held for dev_set lock (Alex) > * Internalize error paths for vfio_device_get_kvm_safe and now return > void - either device->kvm is set with a ref taken or is NULL (Alex) > * Other flow suggestions to make the call path cleaner - Thanks! (Alex) > * Can't pass group->kvm to vfio_device_open, as it references the value > outside of new lock. Pass device->kvm to minimize changes in this > fix (Alex, Yi) > Changes from v1: > * use spin_lock instead of spin_lock_irqsave (Jason) > * clear device->kvm_put as part of vfio_kvm_put_kvm (Yi) > * Re-arrange code to avoid referencing the group contents from within > vfio_main (Kevin) which meant moving most of the code in this patch > to group.c along with getting/dropping of the dev_set lock > --- > drivers/vfio/group.c | 32 ++++++++++++++---- > drivers/vfio/vfio.h | 14 ++++++++ > drivers/vfio/vfio_main.c | 70 > ++++++++++++++++++++++++++++++++++++---- > include/linux/vfio.h | 2 +- > 4 files changed, 103 insertions(+), 15 deletions(-) > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index bb24b2f0271e..7fed4233ca23 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -164,13 +164,23 @@ static int vfio_device_group_open(struct > vfio_device *device) > goto out_unlock; > } > > + mutex_lock(&device->dev_set->lock); > + > /* > - * Here we pass the KVM pointer with the group under the lock. If > the > - * device driver will use it, it must obtain a reference and release it > - * during close_device. > + * Before the first device open, get the KVM pointer currently > + * associated with the group (if there is one) and obtain a reference > + * now that will be held until the open_count reaches 0 again. Save > + * the pointer in the device for use by drivers. > */ > - ret = vfio_device_open(device, device->group->iommufd, > - device->group->kvm); > + if (device->open_count == 0) > + vfio_device_get_kvm_safe(device); > + > + ret = vfio_device_open(device, device->group->iommufd, device- > >kvm); > + > + if (device->open_count == 0) > + vfio_device_put_kvm(device); > + > + mutex_unlock(&device->dev_set->lock); > > out_unlock: > mutex_unlock(&device->group->group_lock); > @@ -180,7 +190,14 @@ static int vfio_device_group_open(struct > vfio_device *device) > void vfio_device_group_close(struct vfio_device *device) > { > mutex_lock(&device->group->group_lock); > + mutex_lock(&device->dev_set->lock); > + > vfio_device_close(device, device->group->iommufd); > + > + if (device->open_count == 0) > + vfio_device_put_kvm(device); > + > + mutex_unlock(&device->dev_set->lock); > mutex_unlock(&device->group->group_lock); > } > > @@ -450,6 +467,7 @@ static struct vfio_group *vfio_group_alloc(struct > iommu_group *iommu_group, > > refcount_set(&group->drivers, 1); > mutex_init(&group->group_lock); > + spin_lock_init(&group->kvm_ref_lock); > INIT_LIST_HEAD(&group->device_list); > mutex_init(&group->device_lock); > group->iommu_group = iommu_group; > @@ -803,9 +821,9 @@ void vfio_file_set_kvm(struct file *file, struct kvm > *kvm) > if (!vfio_file_is_group(file)) > return; > > - mutex_lock(&group->group_lock); > + spin_lock(&group->kvm_ref_lock); > group->kvm = kvm; > - mutex_unlock(&group->group_lock); > + spin_unlock(&group->kvm_ref_lock); > } > EXPORT_SYMBOL_GPL(vfio_file_set_kvm); > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index f8219a438bfb..20d715b0a3a8 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -74,6 +74,7 @@ struct vfio_group { > struct file *opened_file; > struct blocking_notifier_head notifier; > struct iommufd_ctx *iommufd; > + spinlock_t kvm_ref_lock; > }; > > int vfio_device_set_group(struct vfio_device *device, > @@ -251,4 +252,17 @@ extern bool vfio_noiommu __read_mostly; > enum { vfio_noiommu = false }; > #endif > > +#ifdef CONFIG_HAVE_KVM > +void vfio_device_get_kvm_safe(struct vfio_device *device); > +void vfio_device_put_kvm(struct vfio_device *device); > +#else > +static inline void vfio_device_get_kvm_safe(struct vfio_device *device) > +{ > +} > + > +static inline void vfio_device_put_kvm(struct vfio_device *device) > +{ > +} > +#endif > + > #endif > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 5177bb061b17..4762550e9f42 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -16,6 +16,9 @@ > #include <linux/fs.h> > #include <linux/idr.h> > #include <linux/iommu.h> > +#ifdef CONFIG_HAVE_KVM > +#include <linux/kvm_host.h> > +#endif > #include <linux/list.h> > #include <linux/miscdevice.h> > #include <linux/module.h> > @@ -338,6 +341,62 @@ void vfio_unregister_group_dev(struct vfio_device > *device) > } > EXPORT_SYMBOL_GPL(vfio_unregister_group_dev); > > +#ifdef CONFIG_HAVE_KVM > +void vfio_device_get_kvm_safe(struct vfio_device *device) > +{ > + void (*pfn)(struct kvm *kvm); > + bool (*fn)(struct kvm *kvm); > + bool ret; > + > + lockdep_assert_held(&device->dev_set->lock); > + > + spin_lock(&device->group->kvm_ref_lock); > + if (!device->group->kvm) > + goto unlock; > + > + pfn = symbol_get(kvm_put_kvm); > + if (WARN_ON(!pfn)) > + goto unlock; > + > + fn = symbol_get(kvm_get_kvm_safe); > + if (WARN_ON(!fn)) { > + symbol_put(kvm_put_kvm); > + goto unlock; > + } > + > + ret = fn(device->group->kvm); > + symbol_put(kvm_get_kvm_safe); > + if (!ret) { > + symbol_put(kvm_put_kvm); > + goto unlock; > + } > + > + device->put_kvm = pfn; > + device->kvm = device->group->kvm; > + > +unlock: > + spin_unlock(&device->group->kvm_ref_lock); > +} > + > +void vfio_device_put_kvm(struct vfio_device *device) > +{ > + lockdep_assert_held(&device->dev_set->lock); > + > + if (!device->kvm) > + return; > + > + if (WARN_ON(!device->put_kvm)) > + goto clear; > + > + device->put_kvm(device->kvm); > + device->put_kvm = NULL; > + symbol_put(kvm_put_kvm); > + > +clear: > + device->kvm = NULL; > +} > +#endif > + > /* true if the vfio_device has open_device() called but not close_device() > */ > static bool vfio_assert_device_open(struct vfio_device *device) > { > @@ -361,7 +420,6 @@ static int vfio_device_first_open(struct vfio_device > *device, > if (ret) > goto err_module_put; > > - device->kvm = kvm; Since you've deleted the only usage of kvm pointer in this function, I guess you can remove the kvm parameter from vfio_device_open() and vfio_device_first_open(). :-) if it makes this patch too big, may just have another patch to do it. Regards, Yi Liu > if (device->ops->open_device) { > ret = device->ops->open_device(device); > if (ret) > @@ -370,7 +428,6 @@ static int vfio_device_first_open(struct vfio_device > *device, > return 0; > > err_unuse_iommu: > - device->kvm = NULL; > if (iommufd) > vfio_iommufd_unbind(device); > else > @@ -387,7 +444,6 @@ static void vfio_device_last_close(struct vfio_device > *device, > > if (device->ops->close_device) > device->ops->close_device(device); > - device->kvm = NULL; > if (iommufd) > vfio_iommufd_unbind(device); > else > @@ -400,14 +456,14 @@ int vfio_device_open(struct vfio_device *device, > { > int ret = 0; > > - mutex_lock(&device->dev_set->lock); > + lockdep_assert_held(&device->dev_set->lock); > + > device->open_count++; > if (device->open_count == 1) { > ret = vfio_device_first_open(device, iommufd, kvm); > if (ret) > device->open_count--; > } > - mutex_unlock(&device->dev_set->lock); > > return ret; > } > @@ -415,12 +471,12 @@ int vfio_device_open(struct vfio_device *device, > void vfio_device_close(struct vfio_device *device, > struct iommufd_ctx *iommufd) > { > - mutex_lock(&device->dev_set->lock); > + lockdep_assert_held(&device->dev_set->lock); > + > vfio_assert_device_open(device); > if (device->open_count == 1) > vfio_device_last_close(device, iommufd); > device->open_count--; > - mutex_unlock(&device->dev_set->lock); > } > > /* > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 35be78e9ae57..87ff862ff555 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -46,7 +46,6 @@ struct vfio_device { > struct vfio_device_set *dev_set; > struct list_head dev_set_list; > unsigned int migration_flags; > - /* Driver must reference the kvm during open_device or never > touch it */ > struct kvm *kvm; > > /* Members below here are private, not for driver use */ > @@ -58,6 +57,7 @@ struct vfio_device { > struct list_head group_next; > struct list_head iommu_entry; > struct iommufd_access *iommufd_access; > + void (*put_kvm)(struct kvm *kvm); > #if IS_ENABLED(CONFIG_IOMMUFD) > struct iommufd_device *iommufd_device; > struct iommufd_ctx *iommufd_ictx; > -- > 2.39.1
> From: Tian, Kevin <kevin.tian@intel.com> > Sent: Friday, February 3, 2023 10:00 AM > > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Friday, February 3, 2023 7:13 AM > > > > On Thu, 2 Feb 2023 23:04:10 +0000 > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > Sent: Friday, February 3, 2023 3:42 AM > > > > > > > > > > > > LGTM. I'm not sure moving the functions to vfio_main really buys us > > > > anything since we're making so much use of group fields. The cdev > > > > approach will necessarily be different, so the bulk of the get code will > > > > likely need to move back to group.c anyway. > > > > > > > > > > well my last comment was based on Matthew's v2 where the get code > > > gets a kvm passed in instead of implicitly retrieving group ref_lock > > > internally. In that case the get/put helpers only contain device logic > > > thus fit in vfio_main.c. > > > > > > with v3 then they have to be in group.c since we don't want to use > > > group fields in vfio_main.c. > > > > > > but I still think v2 of the helpers is slightly better. The only difference > > > between cdev and group when handling this race is using different > > > ref_lock. the symbol get/put part is exactly same. So even if we > > > merge v3 like this, very likely Yi has to change it back to v2 style > > > to share the get/put helpers while just leaving the ref_lock part > > > handled differently between the two path. > > > > I'm not really a fan of the asymmetry of the v2 version where the get > > helper needs to be called under the new kvm_ref_lock, but the put > > helper does not. Having the get helper handle that makes the caller > > much cleaner. Thanks, > > > > What about passing the lock pointer into the helper? it's still slightly > asymmetry as the put helper doesn't carry the lock pointer but it > could also be interpreted as if the pointer has been saved in the get > then if it needs to be referenced by the put there is no need to pass > it in again. For cdev, I may modify vfio_device_get_kvm_safe() to accept struct kvm and let its caller hold a kvm_ref_lock (field within struct vfio_device_file). Meanwhile, the group path holds the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). vfio_device_get_kvm_safe() just includes the symbol get/put and the device->kvm and put_kvm set. Regards, Yi Liu
On Fri, 3 Feb 2023 13:32:09 +0000 "Liu, Yi L" <yi.l.liu@intel.com> wrote: > > From: Tian, Kevin <kevin.tian@intel.com> > > Sent: Friday, February 3, 2023 10:00 AM > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > Sent: Friday, February 3, 2023 7:13 AM > > > > > > On Thu, 2 Feb 2023 23:04:10 +0000 > > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > > Sent: Friday, February 3, 2023 3:42 AM > > > > > > > > > > > > > > > LGTM. I'm not sure moving the functions to vfio_main really buys us > > > > > anything since we're making so much use of group fields. The cdev > > > > > approach will necessarily be different, so the bulk of the get code will > > > > > likely need to move back to group.c anyway. > > > > > > > > > > > > > well my last comment was based on Matthew's v2 where the get code > > > > gets a kvm passed in instead of implicitly retrieving group ref_lock > > > > internally. In that case the get/put helpers only contain device logic > > > > thus fit in vfio_main.c. > > > > > > > > with v3 then they have to be in group.c since we don't want to use > > > > group fields in vfio_main.c. > > > > > > > > but I still think v2 of the helpers is slightly better. The only difference > > > > between cdev and group when handling this race is using different > > > > ref_lock. the symbol get/put part is exactly same. So even if we > > > > merge v3 like this, very likely Yi has to change it back to v2 style > > > > to share the get/put helpers while just leaving the ref_lock part > > > > handled differently between the two path. > > > > > > I'm not really a fan of the asymmetry of the v2 version where the get > > > helper needs to be called under the new kvm_ref_lock, but the put > > > helper does not. Having the get helper handle that makes the caller > > > much cleaner. Thanks, > > > > > > > What about passing the lock pointer into the helper? it's still slightly > > asymmetry as the put helper doesn't carry the lock pointer but it > > could also be interpreted as if the pointer has been saved in the get > > then if it needs to be referenced by the put there is no need to pass > > it in again. > > For cdev, I may modify vfio_device_get_kvm_safe() to accept > struct kvm and let its caller hold a kvm_ref_lock (field within > struct vfio_device_file). Meanwhile, the group path holds > the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). > vfio_device_get_kvm_safe() just includes the symbol get/put and > the device->kvm and put_kvm set. Sounds a lot like v2 :-\ I'd look more towards group and cdev specific helpers that handle the locking so that the callers aren't exposed to the asymmetry of get vs put, and reduce a new _vfio_device_get_kvm_safe() in common code that only does the symbol work. Thanks, Alex
On 2/3/23 3:58 AM, Liu, Yi L wrote: > Hi Matthew, > ... >> * Can't pass group->kvm to vfio_device_open, as it references the value >> outside of new lock. Pass device->kvm to minimize changes in this >> fix (Alex, Yi) ... >> @@ -361,7 +420,6 @@ static int vfio_device_first_open(struct vfio_device >> *device, >> if (ret) >> goto err_module_put; >> >> - device->kvm = kvm; > > Since you've deleted the only usage of kvm pointer in this function, I > guess you can remove the kvm parameter from vfio_device_open() > and vfio_device_first_open(). :-) if it makes this patch too big, may > just have another patch to do it. > Hi Yi, Yeah, I mentioned it briefly (and vaguely I guess) in the cover, that was intentionally left out to reduce the patch size since this is a fix. I thought that was the consensus from the v2 comments anyway. If I end up doing a v4 for this I can just include the removal as a 2nd patch (without a fixes tag) and Alex can squash or keep separate as preferred -- if not you can feel free to do that removal with your cdev follow-up that exploits this work. Thanks, Matt
> From: Matthew Rosato <mjrosato@linux.ibm.com> > Sent: Friday, February 3, 2023 10:26 PM > > On 2/3/23 3:58 AM, Liu, Yi L wrote: > > Hi Matthew, > > > ... > >> * Can't pass group->kvm to vfio_device_open, as it references the value > >> outside of new lock. Pass device->kvm to minimize changes in this > >> fix (Alex, Yi) > ... > >> @@ -361,7 +420,6 @@ static int vfio_device_first_open(struct > vfio_device > >> *device, > >> if (ret) > >> goto err_module_put; > >> > >> - device->kvm = kvm; > > > > Since you've deleted the only usage of kvm pointer in this function, I > > guess you can remove the kvm parameter from vfio_device_open() > > and vfio_device_first_open(). :-) if it makes this patch too big, may > > just have another patch to do it. > > > > Hi Yi, > > Yeah, I mentioned it briefly (and vaguely I guess) in the cover, that was > intentionally left out to reduce the patch size since this is a fix. I thought > that was the consensus from the v2 comments anyway. > > If I end up doing a v4 for this I can just include the removal as a 2nd patch > (without a fixes tag) and Alex can squash or keep separate as preferred -- if > not you can feel free to do that removal with your cdev follow-up that > exploits this work. Sure.
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Friday, February 3, 2023 9:50 PM > > On Fri, 3 Feb 2023 13:32:09 +0000 > "Liu, Yi L" <yi.l.liu@intel.com> wrote: > > > > From: Tian, Kevin <kevin.tian@intel.com> > > > Sent: Friday, February 3, 2023 10:00 AM > > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > Sent: Friday, February 3, 2023 7:13 AM > > > > > > > > On Thu, 2 Feb 2023 23:04:10 +0000 > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > > > Sent: Friday, February 3, 2023 3:42 AM > > > > > > > > > > > > > > > > > > LGTM. I'm not sure moving the functions to vfio_main really buys > us > > > > > > anything since we're making so much use of group fields. The cdev > > > > > > approach will necessarily be different, so the bulk of the get code > will > > > > > > likely need to move back to group.c anyway. > > > > > > > > > > > > > > > > well my last comment was based on Matthew's v2 where the get > code > > > > > gets a kvm passed in instead of implicitly retrieving group ref_lock > > > > > internally. In that case the get/put helpers only contain device logic > > > > > thus fit in vfio_main.c. > > > > > > > > > > with v3 then they have to be in group.c since we don't want to use > > > > > group fields in vfio_main.c. > > > > > > > > > > but I still think v2 of the helpers is slightly better. The only difference > > > > > between cdev and group when handling this race is using different > > > > > ref_lock. the symbol get/put part is exactly same. So even if we > > > > > merge v3 like this, very likely Yi has to change it back to v2 style > > > > > to share the get/put helpers while just leaving the ref_lock part > > > > > handled differently between the two path. > > > > > > > > I'm not really a fan of the asymmetry of the v2 version where the get > > > > helper needs to be called under the new kvm_ref_lock, but the put > > > > helper does not. Having the get helper handle that makes the caller > > > > much cleaner. Thanks, > > > > > > > > > > What about passing the lock pointer into the helper? it's still slightly > > > asymmetry as the put helper doesn't carry the lock pointer but it > > > could also be interpreted as if the pointer has been saved in the get > > > then if it needs to be referenced by the put there is no need to pass > > > it in again. > > > > For cdev, I may modify vfio_device_get_kvm_safe() to accept > > struct kvm and let its caller hold a kvm_ref_lock (field within > > struct vfio_device_file). Meanwhile, the group path holds > > the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). > > vfio_device_get_kvm_safe() just includes the symbol get/put and > > the device->kvm and put_kvm set. > > Sounds a lot like v2 :-\ Yes, like v2.
On Fri, 3 Feb 2023 14:54:44 +0000 "Liu, Yi L" <yi.l.liu@intel.com> wrote: > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Friday, February 3, 2023 9:50 PM > > > > On Fri, 3 Feb 2023 13:32:09 +0000 > > "Liu, Yi L" <yi.l.liu@intel.com> wrote: > > > > > > From: Tian, Kevin <kevin.tian@intel.com> > > > > Sent: Friday, February 3, 2023 10:00 AM > > > > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > > Sent: Friday, February 3, 2023 7:13 AM > > > > > > > > > > On Thu, 2 Feb 2023 23:04:10 +0000 > > > > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > > > > Sent: Friday, February 3, 2023 3:42 AM > > > > > > > > > > > > > > > > > > > > > LGTM. I'm not sure moving the functions to vfio_main really buys > > us > > > > > > > anything since we're making so much use of group fields. The cdev > > > > > > > approach will necessarily be different, so the bulk of the get code > > will > > > > > > > likely need to move back to group.c anyway. > > > > > > > > > > > > > > > > > > > well my last comment was based on Matthew's v2 where the get > > code > > > > > > gets a kvm passed in instead of implicitly retrieving group ref_lock > > > > > > internally. In that case the get/put helpers only contain device logic > > > > > > thus fit in vfio_main.c. > > > > > > > > > > > > with v3 then they have to be in group.c since we don't want to use > > > > > > group fields in vfio_main.c. > > > > > > > > > > > > but I still think v2 of the helpers is slightly better. The only difference > > > > > > between cdev and group when handling this race is using different > > > > > > ref_lock. the symbol get/put part is exactly same. So even if we > > > > > > merge v3 like this, very likely Yi has to change it back to v2 style > > > > > > to share the get/put helpers while just leaving the ref_lock part > > > > > > handled differently between the two path. > > > > > > > > > > I'm not really a fan of the asymmetry of the v2 version where the get > > > > > helper needs to be called under the new kvm_ref_lock, but the put > > > > > helper does not. Having the get helper handle that makes the caller > > > > > much cleaner. Thanks, > > > > > > > > > > > > > What about passing the lock pointer into the helper? it's still slightly > > > > asymmetry as the put helper doesn't carry the lock pointer but it > > > > could also be interpreted as if the pointer has been saved in the get > > > > then if it needs to be referenced by the put there is no need to pass > > > > it in again. > > > > > > For cdev, I may modify vfio_device_get_kvm_safe() to accept > > > struct kvm and let its caller hold a kvm_ref_lock (field within > > > struct vfio_device_file). Meanwhile, the group path holds > > > the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). > > > vfio_device_get_kvm_safe() just includes the symbol get/put and > > > the device->kvm and put_kvm set. > > > > Sounds a lot like v2 :-\ > > Yes, like v2.
On 2/3/23 10:19 AM, Alex Williamson wrote: > On Fri, 3 Feb 2023 14:54:44 +0000 > "Liu, Yi L" <yi.l.liu@intel.com> wrote: > >>> From: Alex Williamson <alex.williamson@redhat.com> >>> Sent: Friday, February 3, 2023 9:50 PM >>> >>> On Fri, 3 Feb 2023 13:32:09 +0000 >>> "Liu, Yi L" <yi.l.liu@intel.com> wrote: >>> >>>>> From: Tian, Kevin <kevin.tian@intel.com> >>>>> Sent: Friday, February 3, 2023 10:00 AM >>>>> >>>>>> From: Alex Williamson <alex.williamson@redhat.com> >>>>>> Sent: Friday, February 3, 2023 7:13 AM >>>>>> >>>>>> On Thu, 2 Feb 2023 23:04:10 +0000 >>>>>> "Tian, Kevin" <kevin.tian@intel.com> wrote: >>>>>> >>>>>>>> From: Alex Williamson <alex.williamson@redhat.com> >>>>>>>> Sent: Friday, February 3, 2023 3:42 AM >>>>>>>> >>>>>>>> >>>>>>>> LGTM. I'm not sure moving the functions to vfio_main really buys >>> us >>>>>>>> anything since we're making so much use of group fields. The cdev >>>>>>>> approach will necessarily be different, so the bulk of the get code >>> will >>>>>>>> likely need to move back to group.c anyway. >>>>>>>> >>>>>>> >>>>>>> well my last comment was based on Matthew's v2 where the get >>> code >>>>>>> gets a kvm passed in instead of implicitly retrieving group ref_lock >>>>>>> internally. In that case the get/put helpers only contain device logic >>>>>>> thus fit in vfio_main.c. >>>>>>> >>>>>>> with v3 then they have to be in group.c since we don't want to use >>>>>>> group fields in vfio_main.c. >>>>>>> >>>>>>> but I still think v2 of the helpers is slightly better. The only difference >>>>>>> between cdev and group when handling this race is using different >>>>>>> ref_lock. the symbol get/put part is exactly same. So even if we >>>>>>> merge v3 like this, very likely Yi has to change it back to v2 style >>>>>>> to share the get/put helpers while just leaving the ref_lock part >>>>>>> handled differently between the two path. >>>>>> >>>>>> I'm not really a fan of the asymmetry of the v2 version where the get >>>>>> helper needs to be called under the new kvm_ref_lock, but the put >>>>>> helper does not. Having the get helper handle that makes the caller >>>>>> much cleaner. Thanks, >>>>>> >>>>> >>>>> What about passing the lock pointer into the helper? it's still slightly >>>>> asymmetry as the put helper doesn't carry the lock pointer but it >>>>> could also be interpreted as if the pointer has been saved in the get >>>>> then if it needs to be referenced by the put there is no need to pass >>>>> it in again. >>>> >>>> For cdev, I may modify vfio_device_get_kvm_safe() to accept >>>> struct kvm and let its caller hold a kvm_ref_lock (field within >>>> struct vfio_device_file). Meanwhile, the group path holds >>>> the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). >>>> vfio_device_get_kvm_safe() just includes the symbol get/put and >>>> the device->kvm and put_kvm set. >>> >>> Sounds a lot like v2 :-\ >> >> Yes, like v2.
On Fri, 3 Feb 2023 12:29:01 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 2/3/23 10:19 AM, Alex Williamson wrote: > > On Fri, 3 Feb 2023 14:54:44 +0000 > > "Liu, Yi L" <yi.l.liu@intel.com> wrote: > > > >>> From: Alex Williamson <alex.williamson@redhat.com> > >>> Sent: Friday, February 3, 2023 9:50 PM > >>> > >>> On Fri, 3 Feb 2023 13:32:09 +0000 > >>> "Liu, Yi L" <yi.l.liu@intel.com> wrote: > >>> > >>>>> From: Tian, Kevin <kevin.tian@intel.com> > >>>>> Sent: Friday, February 3, 2023 10:00 AM > >>>>> > >>>>>> From: Alex Williamson <alex.williamson@redhat.com> > >>>>>> Sent: Friday, February 3, 2023 7:13 AM > >>>>>> > >>>>>> On Thu, 2 Feb 2023 23:04:10 +0000 > >>>>>> "Tian, Kevin" <kevin.tian@intel.com> wrote: > >>>>>> > >>>>>>>> From: Alex Williamson <alex.williamson@redhat.com> > >>>>>>>> Sent: Friday, February 3, 2023 3:42 AM > >>>>>>>> > >>>>>>>> > >>>>>>>> LGTM. I'm not sure moving the functions to vfio_main really buys > >>> us > >>>>>>>> anything since we're making so much use of group fields. The cdev > >>>>>>>> approach will necessarily be different, so the bulk of the get code > >>> will > >>>>>>>> likely need to move back to group.c anyway. > >>>>>>>> > >>>>>>> > >>>>>>> well my last comment was based on Matthew's v2 where the get > >>> code > >>>>>>> gets a kvm passed in instead of implicitly retrieving group ref_lock > >>>>>>> internally. In that case the get/put helpers only contain device logic > >>>>>>> thus fit in vfio_main.c. > >>>>>>> > >>>>>>> with v3 then they have to be in group.c since we don't want to use > >>>>>>> group fields in vfio_main.c. > >>>>>>> > >>>>>>> but I still think v2 of the helpers is slightly better. The only difference > >>>>>>> between cdev and group when handling this race is using different > >>>>>>> ref_lock. the symbol get/put part is exactly same. So even if we > >>>>>>> merge v3 like this, very likely Yi has to change it back to v2 style > >>>>>>> to share the get/put helpers while just leaving the ref_lock part > >>>>>>> handled differently between the two path. > >>>>>> > >>>>>> I'm not really a fan of the asymmetry of the v2 version where the get > >>>>>> helper needs to be called under the new kvm_ref_lock, but the put > >>>>>> helper does not. Having the get helper handle that makes the caller > >>>>>> much cleaner. Thanks, > >>>>>> > >>>>> > >>>>> What about passing the lock pointer into the helper? it's still slightly > >>>>> asymmetry as the put helper doesn't carry the lock pointer but it > >>>>> could also be interpreted as if the pointer has been saved in the get > >>>>> then if it needs to be referenced by the put there is no need to pass > >>>>> it in again. > >>>> > >>>> For cdev, I may modify vfio_device_get_kvm_safe() to accept > >>>> struct kvm and let its caller hold a kvm_ref_lock (field within > >>>> struct vfio_device_file). Meanwhile, the group path holds > >>>> the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). > >>>> vfio_device_get_kvm_safe() just includes the symbol get/put and > >>>> the device->kvm and put_kvm set. > >>> > >>> Sounds a lot like v2 :-\ > >> > >> Yes, like v2.
On 2/3/23 3:35 PM, Alex Williamson wrote: > On Fri, 3 Feb 2023 12:29:01 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: ... > I'd probably go back to making this: > > void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm); > > so the vfio_main function would handle setting and clearing > device->kvm. That way we could also move the lockdep into the > vfio_main functions. Once we do that, there's no reason to have a > group vs cdev put function and we end up with only a wrapper on the get > function, which should really never be used directly, so we prefix it > with an underscore. > > At that point (see incremental diff below), it's about a wash. Current v3: > > drivers/vfio/group.c | 32 +++++++++++++---- > drivers/vfio/vfio.h | 14 +++++++ > drivers/vfio/vfio_main.c | 70 +++++++++++++++++++++++++++++++++++---- > include/linux/vfio.h | 2 - > 4 files changed, 103 insertions(+), 15 deletions(-) > > Folding in below: > > drivers/vfio/group.c | 44 ++++++++++++++++++++++----- > drivers/vfio/vfio.h | 15 +++++++++ > drivers/vfio/vfio_main.c | 63 ++++++++++++++++++++++++++++++++++----- > include/linux/vfio.h | 2 - > 4 files changed, 109 insertions(+), 15 deletions(-) > > Unfortunately it seems I've talked myself into the answer that we > should maybe just pre-enable cdev by not adding a group reference in > vfio_main. Thanks, > > Alex > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index 7fed4233ca23..98621ac082f0 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -154,6 +154,18 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group, > return ret; > } > > +static void vfio_device_group_get_kvm_safe(struct vfio_device *device) > +{ > + spin_lock(&device->group->kvm_ref_lock); > + if (!device->group->kvm) > + goto unlock; > + > + _vfio_device_get_kvm_safe(device, device->group->kvm); > + > +unlock: > + spin_unlock(&device->group->kvm_ref_lock); > +} > + > static int vfio_device_group_open(struct vfio_device *device) > { > int ret; > @@ -173,7 +185,7 @@ static int vfio_device_group_open(struct vfio_device *device) > * the pointer in the device for use by drivers. > */ > if (device->open_count == 0) > - vfio_device_get_kvm_safe(device); > + vfio_device_group_get_kvm_safe(device); > > ret = vfio_device_open(device, device->group->iommufd, device->kvm); > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index 20d715b0a3a8..24d6cd285945 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -253,10 +253,11 @@ enum { vfio_noiommu = false }; > #endif > > #ifdef CONFIG_HAVE_KVM > -void vfio_device_get_kvm_safe(struct vfio_device *device); > +void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm); > void vfio_device_put_kvm(struct vfio_device *device); > #else > -static inline void vfio_device_get_kvm_safe(struct vfio_device *device) > +static inline void _vfio_device_get_kvm_safe(struct vfio_device *device, > + struct kvm *kvm) > { > } > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 4762550e9f42..00d4d5167d6c 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -342,7 +342,7 @@ void vfio_unregister_group_dev(struct vfio_device *device) > EXPORT_SYMBOL_GPL(vfio_unregister_group_dev); > > #ifdef CONFIG_HAVE_KVM > -void vfio_device_get_kvm_safe(struct vfio_device *device) > +void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm) > { > void (*pfn)(struct kvm *kvm); > bool (*fn)(struct kvm *kvm); > @@ -350,32 +350,25 @@ void vfio_device_get_kvm_safe(struct vfio_device *device) > > lockdep_assert_held(&device->dev_set->lock); > > - spin_lock(&device->group->kvm_ref_lock); > - if (!device->group->kvm) > - goto unlock; > - > pfn = symbol_get(kvm_put_kvm); > if (WARN_ON(!pfn)) > - goto unlock; > + return; > > fn = symbol_get(kvm_get_kvm_safe); > if (WARN_ON(!fn)) { > symbol_put(kvm_put_kvm); > - goto unlock; > + return; > } > > ret = fn(device->group->kvm); s/device->group->kvm/kvm/ With that small change, this looks good to me too (and testing looks good too). Do you want me to send a v4 for one last round of review? Thanks, Matt
On Fri, 3 Feb 2023 16:19:10 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > @@ -350,32 +350,25 @@ void vfio_device_get_kvm_safe(struct vfio_device *device) > > > > lockdep_assert_held(&device->dev_set->lock); > > > > - spin_lock(&device->group->kvm_ref_lock); > > - if (!device->group->kvm) > > - goto unlock; > > - > > pfn = symbol_get(kvm_put_kvm); > > if (WARN_ON(!pfn)) > > - goto unlock; > > + return; > > > > fn = symbol_get(kvm_get_kvm_safe); > > if (WARN_ON(!fn)) { > > symbol_put(kvm_put_kvm); > > - goto unlock; > > + return; > > } > > > ret = fn(device->group->kvm); > > s/device->group->kvm/kvm/ Oops, yes. > With that small change, this looks good to me too (and testing looks > good too). Do you want me to send a v4 for one last round of review? Please do. Thanks, Alex
diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c index bb24b2f0271e..7fed4233ca23 100644 --- a/drivers/vfio/group.c +++ b/drivers/vfio/group.c @@ -164,13 +164,23 @@ static int vfio_device_group_open(struct vfio_device *device) goto out_unlock; } + mutex_lock(&device->dev_set->lock); + /* - * Here we pass the KVM pointer with the group under the lock. If the - * device driver will use it, it must obtain a reference and release it - * during close_device. + * Before the first device open, get the KVM pointer currently + * associated with the group (if there is one) and obtain a reference + * now that will be held until the open_count reaches 0 again. Save + * the pointer in the device for use by drivers. */ - ret = vfio_device_open(device, device->group->iommufd, - device->group->kvm); + if (device->open_count == 0) + vfio_device_get_kvm_safe(device); + + ret = vfio_device_open(device, device->group->iommufd, device->kvm); + + if (device->open_count == 0) + vfio_device_put_kvm(device); + + mutex_unlock(&device->dev_set->lock); out_unlock: mutex_unlock(&device->group->group_lock); @@ -180,7 +190,14 @@ static int vfio_device_group_open(struct vfio_device *device) void vfio_device_group_close(struct vfio_device *device) { mutex_lock(&device->group->group_lock); + mutex_lock(&device->dev_set->lock); + vfio_device_close(device, device->group->iommufd); + + if (device->open_count == 0) + vfio_device_put_kvm(device); + + mutex_unlock(&device->dev_set->lock); mutex_unlock(&device->group->group_lock); } @@ -450,6 +467,7 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group, refcount_set(&group->drivers, 1); mutex_init(&group->group_lock); + spin_lock_init(&group->kvm_ref_lock); INIT_LIST_HEAD(&group->device_list); mutex_init(&group->device_lock); group->iommu_group = iommu_group; @@ -803,9 +821,9 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm) if (!vfio_file_is_group(file)) return; - mutex_lock(&group->group_lock); + spin_lock(&group->kvm_ref_lock); group->kvm = kvm; - mutex_unlock(&group->group_lock); + spin_unlock(&group->kvm_ref_lock); } EXPORT_SYMBOL_GPL(vfio_file_set_kvm); diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index f8219a438bfb..20d715b0a3a8 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -74,6 +74,7 @@ struct vfio_group { struct file *opened_file; struct blocking_notifier_head notifier; struct iommufd_ctx *iommufd; + spinlock_t kvm_ref_lock; }; int vfio_device_set_group(struct vfio_device *device, @@ -251,4 +252,17 @@ extern bool vfio_noiommu __read_mostly; enum { vfio_noiommu = false }; #endif +#ifdef CONFIG_HAVE_KVM +void vfio_device_get_kvm_safe(struct vfio_device *device); +void vfio_device_put_kvm(struct vfio_device *device); +#else +static inline void vfio_device_get_kvm_safe(struct vfio_device *device) +{ +} + +static inline void vfio_device_put_kvm(struct vfio_device *device) +{ +} +#endif + #endif diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 5177bb061b17..4762550e9f42 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -16,6 +16,9 @@ #include <linux/fs.h> #include <linux/idr.h> #include <linux/iommu.h> +#ifdef CONFIG_HAVE_KVM +#include <linux/kvm_host.h> +#endif #include <linux/list.h> #include <linux/miscdevice.h> #include <linux/module.h> @@ -338,6 +341,62 @@ void vfio_unregister_group_dev(struct vfio_device *device) } EXPORT_SYMBOL_GPL(vfio_unregister_group_dev); +#ifdef CONFIG_HAVE_KVM +void vfio_device_get_kvm_safe(struct vfio_device *device) +{ + void (*pfn)(struct kvm *kvm); + bool (*fn)(struct kvm *kvm); + bool ret; + + lockdep_assert_held(&device->dev_set->lock); + + spin_lock(&device->group->kvm_ref_lock); + if (!device->group->kvm) + goto unlock; + + pfn = symbol_get(kvm_put_kvm); + if (WARN_ON(!pfn)) + goto unlock; + + fn = symbol_get(kvm_get_kvm_safe); + if (WARN_ON(!fn)) { + symbol_put(kvm_put_kvm); + goto unlock; + } + + ret = fn(device->group->kvm); + symbol_put(kvm_get_kvm_safe); + if (!ret) { + symbol_put(kvm_put_kvm); + goto unlock; + } + + device->put_kvm = pfn; + device->kvm = device->group->kvm; + +unlock: + spin_unlock(&device->group->kvm_ref_lock); +} + +void vfio_device_put_kvm(struct vfio_device *device) +{ + lockdep_assert_held(&device->dev_set->lock); + + if (!device->kvm) + return; + + if (WARN_ON(!device->put_kvm)) + goto clear; + + device->put_kvm(device->kvm); + device->put_kvm = NULL; + symbol_put(kvm_put_kvm); + +clear: + device->kvm = NULL; +} +#endif + /* true if the vfio_device has open_device() called but not close_device() */ static bool vfio_assert_device_open(struct vfio_device *device) { @@ -361,7 +420,6 @@ static int vfio_device_first_open(struct vfio_device *device, if (ret) goto err_module_put; - device->kvm = kvm; if (device->ops->open_device) { ret = device->ops->open_device(device); if (ret) @@ -370,7 +428,6 @@ static int vfio_device_first_open(struct vfio_device *device, return 0; err_unuse_iommu: - device->kvm = NULL; if (iommufd) vfio_iommufd_unbind(device); else @@ -387,7 +444,6 @@ static void vfio_device_last_close(struct vfio_device *device, if (device->ops->close_device) device->ops->close_device(device); - device->kvm = NULL; if (iommufd) vfio_iommufd_unbind(device); else @@ -400,14 +456,14 @@ int vfio_device_open(struct vfio_device *device, { int ret = 0; - mutex_lock(&device->dev_set->lock); + lockdep_assert_held(&device->dev_set->lock); + device->open_count++; if (device->open_count == 1) { ret = vfio_device_first_open(device, iommufd, kvm); if (ret) device->open_count--; } - mutex_unlock(&device->dev_set->lock); return ret; } @@ -415,12 +471,12 @@ int vfio_device_open(struct vfio_device *device, void vfio_device_close(struct vfio_device *device, struct iommufd_ctx *iommufd) { - mutex_lock(&device->dev_set->lock); + lockdep_assert_held(&device->dev_set->lock); + vfio_assert_device_open(device); if (device->open_count == 1) vfio_device_last_close(device, iommufd); device->open_count--; - mutex_unlock(&device->dev_set->lock); } /* diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 35be78e9ae57..87ff862ff555 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -46,7 +46,6 @@ struct vfio_device { struct vfio_device_set *dev_set; struct list_head dev_set_list; unsigned int migration_flags; - /* Driver must reference the kvm during open_device or never touch it */ struct kvm *kvm; /* Members below here are private, not for driver use */ @@ -58,6 +57,7 @@ struct vfio_device { struct list_head group_next; struct list_head iommu_entry; struct iommufd_access *iommufd_access; + void (*put_kvm)(struct kvm *kvm); #if IS_ENABLED(CONFIG_IOMMUFD) struct iommufd_device *iommufd_device; struct iommufd_ctx *iommufd_ictx;
After 51cdc8bc120e, we have another deadlock scenario between the kvm->lock and the vfio group_lock with two different codepaths acquiring the locks in different order. Specifically in vfio_open_device, vfio holds the vfio group_lock when issuing device->ops->open_device but some drivers (like vfio-ap) need to acquire kvm->lock during their open_device routine; Meanwhile, kvm_vfio_release will acquire the kvm->lock first before calling vfio_file_set_kvm which will acquire the vfio group_lock. To resolve this, let's remove the need for the vfio group_lock from the kvm_vfio_release codepath. This is done by introducing a new spinlock to protect modifications to the vfio group kvm pointer, and acquiring a kvm ref from within vfio while holding this spinlock, with the reference held until the last close for the device in question. Fixes: 51cdc8bc120e ("kvm/vfio: Fix potential deadlock on vfio group_lock") Reported-by: Anthony Krowiak <akrowiak@linux.ibm.com> Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- Changes from v2: * Relocate the new functions back to vfio_main and externalize to call from group (Kevin) since cdev will need this too * s/vfio_kvm_*_kvm/vfio_device_*_kvm/ and only pass device as input. Handle new kvm_ref_lock directly inside vfio_device_get_kvm (Alex) * Add assert_lockdep_held for dev_set lock (Alex) * Internalize error paths for vfio_device_get_kvm_safe and now return void - either device->kvm is set with a ref taken or is NULL (Alex) * Other flow suggestions to make the call path cleaner - Thanks! (Alex) * Can't pass group->kvm to vfio_device_open, as it references the value outside of new lock. Pass device->kvm to minimize changes in this fix (Alex, Yi) Changes from v1: * use spin_lock instead of spin_lock_irqsave (Jason) * clear device->kvm_put as part of vfio_kvm_put_kvm (Yi) * Re-arrange code to avoid referencing the group contents from within vfio_main (Kevin) which meant moving most of the code in this patch to group.c along with getting/dropping of the dev_set lock --- drivers/vfio/group.c | 32 ++++++++++++++---- drivers/vfio/vfio.h | 14 ++++++++ drivers/vfio/vfio_main.c | 70 ++++++++++++++++++++++++++++++++++++---- include/linux/vfio.h | 2 +- 4 files changed, 103 insertions(+), 15 deletions(-)