All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH v3] vfio: fix potential deadlock on vfio group lock
@ 2023-01-13 17:11 ` Matthew Rosato
  0 siblings, 0 replies; 10+ messages in thread
From: Matthew Rosato @ 2023-01-13 17:11 UTC (permalink / raw)
  To: alex.williamson, pbonzini
  Cc: akrowiak, jjherne, farman, imbrenda, frankja, pmorel, david,
	seanjc, intel-gfx, cohuck, linux-kernel, pasic, jgg, kvm,
	linux-s390, borntraeger, intel-gvt-dev

Currently it is possible that the final put of a KVM reference comes from
vfio during its device close operation.  This occurs while the vfio group
lock is held; however, if the vfio device is still in the kvm device list,
then the following call chain could result in a deadlock:

kvm_put_kvm
 -> kvm_destroy_vm
  -> kvm_destroy_devices
   -> kvm_vfio_destroy
    -> kvm_vfio_file_set_kvm
     -> vfio_file_set_kvm
      -> group->group_lock/group_rwsem

Avoid this scenario by having vfio core code acquire a KVM reference
the first time a device is opened and hold that reference until the
device fd is closed, at a point after the group lock has been released.

Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
Changes from v2:
* Re-arrange vfio_kvm_set_kvm_safe error path to still trigger
  device_open with device->kvm=NULL (Alex)
* get device->dev_set->lock when checking device->open_count (Alex)
* but don't hold it over the kvm_put_kvm (Jason)
* get kvm_put symbol upfront and stash it in device until close (Jason)
* check CONFIG_HAVE_KVM to avoid build errors on architectures without
  KVM support

Changes from v1:
* Re-write using symbol get logic to get kvm ref during first device
  open, release the ref during device fd close after group lock is
  released
* Drop kvm get/put changes to drivers; now that vfio core holds a
  kvm ref until sometime after the device_close op is called, it
  should be fine for drivers to get and put their own references to it.
---
 drivers/vfio/group.c     |  6 ++--
 drivers/vfio/vfio_main.c | 78 +++++++++++++++++++++++++++++++++++++---
 include/linux/vfio.h     |  2 +-
 3 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index bb24b2f0271e..2b0da82f82f4 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -165,9 +165,9 @@ static int vfio_device_group_open(struct vfio_device *device)
 	}
 
 	/*
-	 * 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.
+	 * Here we pass the KVM pointer with the group under the lock.  A
+	 * reference will be obtained the first time the device is opened and
+	 * will be held until the device fd is closed.
 	 */
 	ret = vfio_device_open(device, device->group->iommufd,
 			       device->group->kvm);
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 5177bb061b17..dbdf16903d52 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>
@@ -344,6 +347,57 @@ static bool vfio_assert_device_open(struct vfio_device *device)
 	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
 }
 
+#ifdef CONFIG_HAVE_KVM
+static bool vfio_kvm_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
+{
+	void (*pfn)(struct kvm *kvm);
+	bool (*fn)(struct kvm *kvm);
+	bool ret;
+
+	pfn = symbol_get(kvm_put_kvm);
+	if (WARN_ON(!pfn))
+		return false;
+
+	fn = symbol_get(kvm_get_kvm_safe);
+	if (WARN_ON(!fn)) {
+		symbol_put(kvm_put_kvm);
+		return false;
+	}
+
+	ret = fn(kvm);
+	if (ret)
+		device->put_kvm = pfn;
+	else
+		symbol_put(kvm_put_kvm);
+
+	symbol_put(kvm_get_kvm_safe);
+
+	return ret;
+}
+
+static void vfio_kvm_put_kvm(struct vfio_device *device, struct kvm *kvm)
+{
+	if (WARN_ON(!device->put_kvm))
+		return;
+
+	device->put_kvm(kvm);
+
+	device->put_kvm = NULL;
+
+	symbol_put(kvm_put_kvm);
+}
+#else
+static bool vfio_kvm_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
+{
+	return false;
+}
+
+static void vfio_kvm_put_kvm(struct vfio_device *device, struct kvm *kvm)
+{
+
+}
+#endif
+
 static int vfio_device_first_open(struct vfio_device *device,
 				  struct iommufd_ctx *iommufd, struct kvm *kvm)
 {
@@ -361,16 +415,21 @@ static int vfio_device_first_open(struct vfio_device *device,
 	if (ret)
 		goto err_module_put;
 
-	device->kvm = kvm;
+	if (kvm && vfio_kvm_get_kvm_safe(device, kvm))
+		device->kvm = kvm;
+
 	if (device->ops->open_device) {
 		ret = device->ops->open_device(device);
 		if (ret)
-			goto err_unuse_iommu;
+			goto err_put_kvm;
 	}
 	return 0;
 
-err_unuse_iommu:
-	device->kvm = NULL;
+err_put_kvm:
+	if (device->kvm) {
+		vfio_kvm_put_kvm(device, device->kvm);
+		device->kvm = NULL;
+	}
 	if (iommufd)
 		vfio_iommufd_unbind(device);
 	else
@@ -387,7 +446,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
@@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
 static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 {
 	struct vfio_device *device = filep->private_data;
+	struct kvm *kvm = NULL;
 
 	vfio_device_group_close(device);
 
+	mutex_lock(&device->dev_set->lock);
+	if (device->open_count == 0 && device->kvm) {
+		kvm = device->kvm;
+		device->kvm = NULL;
+	}
+	mutex_unlock(&device->dev_set->lock);
+	if (kvm)
+		vfio_kvm_put_kvm(device, kvm);
+
 	vfio_device_put_registration(device);
 
 	return 0;
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.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3] vfio: fix potential deadlock on vfio group lock
@ 2023-01-13 17:11 ` Matthew Rosato
  0 siblings, 0 replies; 10+ messages in thread
From: Matthew Rosato @ 2023-01-13 17:11 UTC (permalink / raw)
  To: alex.williamson, pbonzini
  Cc: jgg, cohuck, farman, pmorel, borntraeger, frankja, imbrenda,
	david, akrowiak, jjherne, pasic, zhenyuw, zhi.a.wang, seanjc,
	linux-s390, kvm, intel-gvt-dev, intel-gfx, linux-kernel

Currently it is possible that the final put of a KVM reference comes from
vfio during its device close operation.  This occurs while the vfio group
lock is held; however, if the vfio device is still in the kvm device list,
then the following call chain could result in a deadlock:

kvm_put_kvm
 -> kvm_destroy_vm
  -> kvm_destroy_devices
   -> kvm_vfio_destroy
    -> kvm_vfio_file_set_kvm
     -> vfio_file_set_kvm
      -> group->group_lock/group_rwsem

Avoid this scenario by having vfio core code acquire a KVM reference
the first time a device is opened and hold that reference until the
device fd is closed, at a point after the group lock has been released.

Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
Changes from v2:
* Re-arrange vfio_kvm_set_kvm_safe error path to still trigger
  device_open with device->kvm=NULL (Alex)
* get device->dev_set->lock when checking device->open_count (Alex)
* but don't hold it over the kvm_put_kvm (Jason)
* get kvm_put symbol upfront and stash it in device until close (Jason)
* check CONFIG_HAVE_KVM to avoid build errors on architectures without
  KVM support

Changes from v1:
* Re-write using symbol get logic to get kvm ref during first device
  open, release the ref during device fd close after group lock is
  released
* Drop kvm get/put changes to drivers; now that vfio core holds a
  kvm ref until sometime after the device_close op is called, it
  should be fine for drivers to get and put their own references to it.
---
 drivers/vfio/group.c     |  6 ++--
 drivers/vfio/vfio_main.c | 78 +++++++++++++++++++++++++++++++++++++---
 include/linux/vfio.h     |  2 +-
 3 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index bb24b2f0271e..2b0da82f82f4 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -165,9 +165,9 @@ static int vfio_device_group_open(struct vfio_device *device)
 	}
 
 	/*
-	 * 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.
+	 * Here we pass the KVM pointer with the group under the lock.  A
+	 * reference will be obtained the first time the device is opened and
+	 * will be held until the device fd is closed.
 	 */
 	ret = vfio_device_open(device, device->group->iommufd,
 			       device->group->kvm);
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 5177bb061b17..dbdf16903d52 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>
@@ -344,6 +347,57 @@ static bool vfio_assert_device_open(struct vfio_device *device)
 	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
 }
 
+#ifdef CONFIG_HAVE_KVM
+static bool vfio_kvm_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
+{
+	void (*pfn)(struct kvm *kvm);
+	bool (*fn)(struct kvm *kvm);
+	bool ret;
+
+	pfn = symbol_get(kvm_put_kvm);
+	if (WARN_ON(!pfn))
+		return false;
+
+	fn = symbol_get(kvm_get_kvm_safe);
+	if (WARN_ON(!fn)) {
+		symbol_put(kvm_put_kvm);
+		return false;
+	}
+
+	ret = fn(kvm);
+	if (ret)
+		device->put_kvm = pfn;
+	else
+		symbol_put(kvm_put_kvm);
+
+	symbol_put(kvm_get_kvm_safe);
+
+	return ret;
+}
+
+static void vfio_kvm_put_kvm(struct vfio_device *device, struct kvm *kvm)
+{
+	if (WARN_ON(!device->put_kvm))
+		return;
+
+	device->put_kvm(kvm);
+
+	device->put_kvm = NULL;
+
+	symbol_put(kvm_put_kvm);
+}
+#else
+static bool vfio_kvm_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
+{
+	return false;
+}
+
+static void vfio_kvm_put_kvm(struct vfio_device *device, struct kvm *kvm)
+{
+
+}
+#endif
+
 static int vfio_device_first_open(struct vfio_device *device,
 				  struct iommufd_ctx *iommufd, struct kvm *kvm)
 {
@@ -361,16 +415,21 @@ static int vfio_device_first_open(struct vfio_device *device,
 	if (ret)
 		goto err_module_put;
 
-	device->kvm = kvm;
+	if (kvm && vfio_kvm_get_kvm_safe(device, kvm))
+		device->kvm = kvm;
+
 	if (device->ops->open_device) {
 		ret = device->ops->open_device(device);
 		if (ret)
-			goto err_unuse_iommu;
+			goto err_put_kvm;
 	}
 	return 0;
 
-err_unuse_iommu:
-	device->kvm = NULL;
+err_put_kvm:
+	if (device->kvm) {
+		vfio_kvm_put_kvm(device, device->kvm);
+		device->kvm = NULL;
+	}
 	if (iommufd)
 		vfio_iommufd_unbind(device);
 	else
@@ -387,7 +446,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
@@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
 static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 {
 	struct vfio_device *device = filep->private_data;
+	struct kvm *kvm = NULL;
 
 	vfio_device_group_close(device);
 
+	mutex_lock(&device->dev_set->lock);
+	if (device->open_count == 0 && device->kvm) {
+		kvm = device->kvm;
+		device->kvm = NULL;
+	}
+	mutex_unlock(&device->dev_set->lock);
+	if (kvm)
+		vfio_kvm_put_kvm(device, kvm);
+
 	vfio_device_put_registration(device);
 
 	return 0;
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.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for vfio: fix potential deadlock on vfio group lock (rev2)
  2023-01-13 17:11 ` Matthew Rosato
  (?)
@ 2023-01-13 17:54 ` Patchwork
  -1 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-01-13 17:54 UTC (permalink / raw)
  To: Matthew Rosato; +Cc: intel-gfx

== Series Details ==

Series: vfio: fix potential deadlock on vfio group lock (rev2)
URL   : https://patchwork.freedesktop.org/series/112759/
State : warning

== Summary ==

Error: dim checkpatch failed
3ddf9fe96184 vfio: fix potential deadlock on vfio group lock
-:109: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
#109: FILE: drivers/vfio/vfio_main.c:397:
+{
+

-:110: CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#110: FILE: drivers/vfio/vfio_main.c:398:
+
+}

total: 0 errors, 0 warnings, 2 checks, 143 lines checked



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH v3] vfio: fix potential deadlock on vfio group lock
  2023-01-13 17:11 ` Matthew Rosato
@ 2023-01-13 18:52   ` Jason Gunthorpe
  -1 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2023-01-13 18:52 UTC (permalink / raw)
  To: Matthew Rosato
  Cc: akrowiak, jjherne, farman, imbrenda, frankja, pmorel, david,
	seanjc, intel-gfx, cohuck, linux-kernel, pasic, kvm, pbonzini,
	linux-s390, borntraeger, intel-gvt-dev

On Fri, Jan 13, 2023 at 12:11:32PM -0500, Matthew Rosato wrote:
> @@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
>  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  {
>  	struct vfio_device *device = filep->private_data;
> +	struct kvm *kvm = NULL;
>  
>  	vfio_device_group_close(device);
>  
> +	mutex_lock(&device->dev_set->lock);
> +	if (device->open_count == 0 && device->kvm) {
> +		kvm = device->kvm;
> +		device->kvm = NULL;
> +	}
> +	mutex_unlock(&device->dev_set->lock);

This still doesn't seem right, another thread could have incr'd the
open_count already 

This has to be done at the moment open_count is decremented to zero,
while still under the original lock.

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] vfio: fix potential deadlock on vfio group lock
@ 2023-01-13 18:52   ` Jason Gunthorpe
  0 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2023-01-13 18:52 UTC (permalink / raw)
  To: Matthew Rosato
  Cc: alex.williamson, pbonzini, cohuck, farman, pmorel, borntraeger,
	frankja, imbrenda, david, akrowiak, jjherne, pasic, zhenyuw,
	zhi.a.wang, seanjc, linux-s390, kvm, intel-gvt-dev, intel-gfx,
	linux-kernel

On Fri, Jan 13, 2023 at 12:11:32PM -0500, Matthew Rosato wrote:
> @@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
>  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  {
>  	struct vfio_device *device = filep->private_data;
> +	struct kvm *kvm = NULL;
>  
>  	vfio_device_group_close(device);
>  
> +	mutex_lock(&device->dev_set->lock);
> +	if (device->open_count == 0 && device->kvm) {
> +		kvm = device->kvm;
> +		device->kvm = NULL;
> +	}
> +	mutex_unlock(&device->dev_set->lock);

This still doesn't seem right, another thread could have incr'd the
open_count already 

This has to be done at the moment open_count is decremented to zero,
while still under the original lock.

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH v3] vfio: fix potential deadlock on vfio group lock
  2023-01-13 18:52   ` Jason Gunthorpe
@ 2023-01-13 20:09     ` Matthew Rosato
  -1 siblings, 0 replies; 10+ messages in thread
From: Matthew Rosato @ 2023-01-13 20:09 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: akrowiak, jjherne, farman, imbrenda, frankja, pmorel, david,
	seanjc, intel-gfx, cohuck, linux-kernel, pasic, kvm, pbonzini,
	linux-s390, borntraeger, intel-gvt-dev

On 1/13/23 1:52 PM, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2023 at 12:11:32PM -0500, Matthew Rosato wrote:
>> @@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
>>  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>>  {
>>  	struct vfio_device *device = filep->private_data;
>> +	struct kvm *kvm = NULL;
>>  
>>  	vfio_device_group_close(device);
>>  
>> +	mutex_lock(&device->dev_set->lock);
>> +	if (device->open_count == 0 && device->kvm) {
>> +		kvm = device->kvm;
>> +		device->kvm = NULL;
>> +	}
>> +	mutex_unlock(&device->dev_set->lock);
> 
> This still doesn't seem right, another thread could have incr'd the
> open_count already 
> 
> This has to be done at the moment open_count is decremented to zero,
> while still under the original lock.

Hmm..  Fair.  Well, we can go back to clearing of device->kvm in vfio_device_last_close but the group lock is held then so we can't immediately do the kvm_put at that time -- unless we go back to the notion of stacking the kvm_put on a workqueue, but now from vfio.  If we do that, I think we also have to scrap the idea of putting the kvm_put_kvm function pointer into device->put_kvm too (or otherwise stash it along with the kvm value to be picked up by the scheduled work).

Another thought would be passing the device->open_count that was read while holding the dev_set->lock back on vfio_close_device() / vfio_device_group_close() as an indicator of whether vfio_device_last_close() was called - then you could use the stashed kvm value because it doesn't matter what's currently in device->kvm or what the current device->open_count is, you know that kvm reference needs to be put.

e.g.:
struct *kvm = device->kvm;
void (*put)(struct kvm *kvm) = device->put_kvm;
opened = vfio_device_group_close(device);
if (opened == 0 && kvm)
	put(kvm);


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] vfio: fix potential deadlock on vfio group lock
@ 2023-01-13 20:09     ` Matthew Rosato
  0 siblings, 0 replies; 10+ messages in thread
From: Matthew Rosato @ 2023-01-13 20:09 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: alex.williamson, pbonzini, cohuck, farman, pmorel, borntraeger,
	frankja, imbrenda, david, akrowiak, jjherne, pasic, zhenyuw,
	zhi.a.wang, seanjc, linux-s390, kvm, intel-gvt-dev, intel-gfx,
	linux-kernel

On 1/13/23 1:52 PM, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2023 at 12:11:32PM -0500, Matthew Rosato wrote:
>> @@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
>>  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>>  {
>>  	struct vfio_device *device = filep->private_data;
>> +	struct kvm *kvm = NULL;
>>  
>>  	vfio_device_group_close(device);
>>  
>> +	mutex_lock(&device->dev_set->lock);
>> +	if (device->open_count == 0 && device->kvm) {
>> +		kvm = device->kvm;
>> +		device->kvm = NULL;
>> +	}
>> +	mutex_unlock(&device->dev_set->lock);
> 
> This still doesn't seem right, another thread could have incr'd the
> open_count already 
> 
> This has to be done at the moment open_count is decremented to zero,
> while still under the original lock.

Hmm..  Fair.  Well, we can go back to clearing of device->kvm in vfio_device_last_close but the group lock is held then so we can't immediately do the kvm_put at that time -- unless we go back to the notion of stacking the kvm_put on a workqueue, but now from vfio.  If we do that, I think we also have to scrap the idea of putting the kvm_put_kvm function pointer into device->put_kvm too (or otherwise stash it along with the kvm value to be picked up by the scheduled work).

Another thought would be passing the device->open_count that was read while holding the dev_set->lock back on vfio_close_device() / vfio_device_group_close() as an indicator of whether vfio_device_last_close() was called - then you could use the stashed kvm value because it doesn't matter what's currently in device->kvm or what the current device->open_count is, you know that kvm reference needs to be put.

e.g.:
struct *kvm = device->kvm;
void (*put)(struct kvm *kvm) = device->put_kvm;
opened = vfio_device_group_close(device);
if (opened == 0 && kvm)
	put(kvm);


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [PATCH v3] vfio: fix potential deadlock on vfio group lock
  2023-01-13 20:09     ` Matthew Rosato
@ 2023-01-13 20:18       ` Jason Gunthorpe
  -1 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2023-01-13 20:18 UTC (permalink / raw)
  To: Matthew Rosato
  Cc: akrowiak, jjherne, farman, imbrenda, frankja, pmorel, david,
	seanjc, intel-gfx, cohuck, linux-kernel, pasic, kvm, pbonzini,
	linux-s390, borntraeger, intel-gvt-dev

On Fri, Jan 13, 2023 at 03:09:01PM -0500, Matthew Rosato wrote:

> > This still doesn't seem right, another thread could have incr'd the
> > open_count already 
> > 
> > This has to be done at the moment open_count is decremented to zero,
> > while still under the original lock.
> 
> Hmm..  Fair.  Well, we can go back to clearing of device->kvm in
> vfio_device_last_close but the group lock is held then so we can't
> immediately do the kvm_put at that time -- unless we go back to the
> notion of stacking the kvm_put on a workqueue, but now from vfio.
> If we do that, I think we also have to scrap the idea of putting the
> kvm_put_kvm function pointer into device->put_kvm too (or otherwise
> stash it along with the kvm value to be picked up by the scheduled
> work).

Well, you have to keep the same sort of design, the
vfio_device_last_close() has to put the kvm on the stack until the
group lock is unlocked.

It is messy due to how the functions are nested, but not hard.

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] vfio: fix potential deadlock on vfio group lock
@ 2023-01-13 20:18       ` Jason Gunthorpe
  0 siblings, 0 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2023-01-13 20:18 UTC (permalink / raw)
  To: Matthew Rosato
  Cc: alex.williamson, pbonzini, cohuck, farman, pmorel, borntraeger,
	frankja, imbrenda, david, akrowiak, jjherne, pasic, zhenyuw,
	zhi.a.wang, seanjc, linux-s390, kvm, intel-gvt-dev, intel-gfx,
	linux-kernel

On Fri, Jan 13, 2023 at 03:09:01PM -0500, Matthew Rosato wrote:

> > This still doesn't seem right, another thread could have incr'd the
> > open_count already 
> > 
> > This has to be done at the moment open_count is decremented to zero,
> > while still under the original lock.
> 
> Hmm..  Fair.  Well, we can go back to clearing of device->kvm in
> vfio_device_last_close but the group lock is held then so we can't
> immediately do the kvm_put at that time -- unless we go back to the
> notion of stacking the kvm_put on a workqueue, but now from vfio.
> If we do that, I think we also have to scrap the idea of putting the
> kvm_put_kvm function pointer into device->put_kvm too (or otherwise
> stash it along with the kvm value to be picked up by the scheduled
> work).

Well, you have to keep the same sort of design, the
vfio_device_last_close() has to put the kvm on the stack until the
group lock is unlocked.

It is messy due to how the functions are nested, but not hard.

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for vfio: fix potential deadlock on vfio group lock (rev2)
  2023-01-13 17:11 ` Matthew Rosato
                   ` (2 preceding siblings ...)
  (?)
@ 2023-01-13 23:47 ` Patchwork
  -1 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-01-13 23:47 UTC (permalink / raw)
  To: Matthew Rosato; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 4167 bytes --]

== Series Details ==

Series: vfio: fix potential deadlock on vfio group lock (rev2)
URL   : https://patchwork.freedesktop.org/series/112759/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12583 -> Patchwork_112759v2
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/index.html

Participating hosts (44 -> 43)
------------------------------

  Additional (1): fi-kbl-8809g 
  Missing    (2): fi-kbl-soraka fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_112759v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#2190])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/fi-kbl-8809g/igt@gem_huc_copy@huc-copy.html

  * igt@i915_module_load@reload:
    - fi-kbl-8809g:       NOTRUN -> [DMESG-WARN][2] ([i915#6899])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/fi-kbl-8809g/igt@i915_module_load@reload.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][3] ([fdo#109271]) +32 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/fi-kbl-8809g/igt@i915_pm_backlight@basic-brightness.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       [PASS][4] -> [FAIL][5] ([i915#6298])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12583/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  * igt@runner@aborted:
    - fi-kbl-8809g:       NOTRUN -> [FAIL][6] ([i915#4312] / [i915#4991])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/fi-kbl-8809g/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@slpc:
    - {bat-adlp-9}:       [DMESG-FAIL][7] ([i915#6367]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12583/bat-adlp-9/igt@i915_selftest@live@slpc.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/bat-adlp-9/igt@i915_selftest@live@slpc.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-n3050:       [FAIL][9] ([i915#6298]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12583/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6899]: https://gitlab.freedesktop.org/drm/intel/issues/6899


Build changes
-------------

  * Linux: CI_DRM_12583 -> Patchwork_112759v2

  CI-20190529: 20190529
  CI_DRM_12583: cb00fa40d334dc6cd0b4894cea5488a4491f5cda @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7119: 1e6d24e6dfa42b22f950f7d5e436b8f9acf8747f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112759v2: cb00fa40d334dc6cd0b4894cea5488a4491f5cda @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

8f01896ce126 vfio: fix potential deadlock on vfio group lock

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112759v2/index.html

[-- Attachment #2: Type: text/html, Size: 5021 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-01-13 23:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-13 17:11 [Intel-gfx] [PATCH v3] vfio: fix potential deadlock on vfio group lock Matthew Rosato
2023-01-13 17:11 ` Matthew Rosato
2023-01-13 17:54 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for vfio: fix potential deadlock on vfio group lock (rev2) Patchwork
2023-01-13 18:52 ` [Intel-gfx] [PATCH v3] vfio: fix potential deadlock on vfio group lock Jason Gunthorpe
2023-01-13 18:52   ` Jason Gunthorpe
2023-01-13 20:09   ` [Intel-gfx] " Matthew Rosato
2023-01-13 20:09     ` Matthew Rosato
2023-01-13 20:18     ` [Intel-gfx] " Jason Gunthorpe
2023-01-13 20:18       ` Jason Gunthorpe
2023-01-13 23:47 ` [Intel-gfx] ✓ Fi.CI.BAT: success for vfio: fix potential deadlock on vfio group lock (rev2) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.