[PATCH v4 0/3] vfio/pci: Check the device set open_count on reset

[PATCH v4 0/3] vfio/pci: Check the device set open_count on reset

[Anthony DeRossi]

I took Alex's suggestion on the v2 patch to add an open count to the device
set. This simplified the final patch and might generally be useful for other
drivers.

The first patch in the series fixes a life cycle issue that was discovered
while adding open_count to vfio_device_set. It is also fixed by a patch [1] in
Jason's "Connect VFIO to IOMMU" series.

The third patch is a rework of the original fix that uses the new device set
open_count instead of the private open_count in vfio_device.

[1] https://lore.kernel.org/kvm/1-v1-4991695894d8+211-vfio_iommufd_jgg@nvidia.com/

Anthony

v3 -> v4:
- Added a patch to fix device registration life cycle
- Added a patch to add a public open_count on vfio_device_set
- Changed the implementation to avoid private open_count usage
v3: https://lore.kernel.org/kvm/20221102055732.2110-1-ajderossi@gmail.com/

v2 -> v3:
- Added WARN_ON()
- Revised commit message
v2: https://lore.kernel.org/kvm/20221026194245.1769-1-ajderossi@gmail.com/

v1 -> v2:
- Changed reset behavior instead of open_count ordering
- Retitled from "vfio: Decrement open_count before close_device()"
v1: https://lore.kernel.org/kvm/20221025193820.4412-1-ajderossi@gmail.com/

Anthony DeRossi (3):
  vfio: Fix container device registration life cycle
  vfio: Add an open counter to vfio_device_set
  vfio/pci: Check the device set open_count on reset

 drivers/vfio/pci/vfio_pci_core.c | 10 +++++-----
 drivers/vfio/vfio_main.c         | 16 +++++++++++-----
 include/linux/vfio.h             |  1 +
 3 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.37.4

[PATCH v4 1/3] vfio: Fix container device registration life cycle

[Anthony DeRossi]

In vfio_device_open(), vfio_container_device_register() is always called
when open_count == 1. On error, vfio_device_container_unregister() is
only called when open_count == 1 and close_device is set. This leaks a
registration for devices without a close_device implementation.

In vfio_device_fops_release(), vfio_device_container_unregister() is
called unconditionally. This can cause a device to be unregistered
multiple times.

Treating container device registration/unregistration uniformly (always
when open_count == 1) fixes both issues.

Fixes: ce4b4657ff18 ("vfio: Replace the DMA unmapping notifier with a callback")
Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
---
 drivers/vfio/vfio_main.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 2d168793d4e1..9a4af880e941 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -801,8 +801,9 @@ static struct file *vfio_device_open(struct vfio_device *device)
 err_close_device:
 	mutex_lock(&device->dev_set->lock);
 	mutex_lock(&device->group->group_lock);
-	if (device->open_count == 1 && device->ops->close_device) {
-		device->ops->close_device(device);
+	if (device->open_count == 1) {
+		if (device->ops->close_device)
+			device->ops->close_device(device);
 
 		vfio_device_container_unregister(device);
 	}
@@ -1017,10 +1018,12 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 	mutex_lock(&device->dev_set->lock);
 	vfio_assert_device_open(device);
 	mutex_lock(&device->group->group_lock);
-	if (device->open_count == 1 && device->ops->close_device)
-		device->ops->close_device(device);
+	if (device->open_count == 1) {
+		if (device->ops->close_device)
+			device->ops->close_device(device);
 
-	vfio_device_container_unregister(device);
+		vfio_device_container_unregister(device);
+	}
 	mutex_unlock(&device->group->group_lock);
 	device->open_count--;
 	if (device->open_count == 0)
-- 
2.37.4

Re: [PATCH v4 1/3] vfio: Fix container device registration life cycle

[Alex Williamson]

On Fri,  4 Nov 2022 12:57:25 -0700
Anthony DeRossi <ajderossi@gmail.com> wrote:

> In vfio_device_open(), vfio_container_device_register() is always called
> when open_count == 1. On error, vfio_device_container_unregister() is
> only called when open_count == 1 and close_device is set. This leaks a
> registration for devices without a close_device implementation.
> 
> In vfio_device_fops_release(), vfio_device_container_unregister() is
> called unconditionally. This can cause a device to be unregistered
> multiple times.
> 
> Treating container device registration/unregistration uniformly (always
> when open_count == 1) fixes both issues.

Good catch, I see that Jason does subtly fix this in "vfio: Move
vfio_device driver open/close code to a function", but I'd rather see
it more overtly fixed in a discrete patch like this.  All "real"
drivers provide a close_device callback, but mdpy and mtty do not.
Thanks,

Alex

> Fixes: ce4b4657ff18 ("vfio: Replace the DMA unmapping notifier with a callback")
> Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
> ---
>  drivers/vfio/vfio_main.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 2d168793d4e1..9a4af880e941 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -801,8 +801,9 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  err_close_device:
>  	mutex_lock(&device->dev_set->lock);
>  	mutex_lock(&device->group->group_lock);
> -	if (device->open_count == 1 && device->ops->close_device) {
> -		device->ops->close_device(device);
> +	if (device->open_count == 1) {
> +		if (device->ops->close_device)
> +			device->ops->close_device(device);
>  
>  		vfio_device_container_unregister(device);
>  	}
> @@ -1017,10 +1018,12 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  	mutex_lock(&device->dev_set->lock);
>  	vfio_assert_device_open(device);
>  	mutex_lock(&device->group->group_lock);
> -	if (device->open_count == 1 && device->ops->close_device)
> -		device->ops->close_device(device);
> +	if (device->open_count == 1) {
> +		if (device->ops->close_device)
> +			device->ops->close_device(device);
>  
> -	vfio_device_container_unregister(device);
> +		vfio_device_container_unregister(device);
> +	}
>  	mutex_unlock(&device->group->group_lock);
>  	device->open_count--;
>  	if (device->open_count == 0)

Re: [PATCH v4 1/3] vfio: Fix container device registration life cycle

[Jason Gunthorpe]

On Fri, Nov 04, 2022 at 02:59:15PM -0600, Alex Williamson wrote:
> On Fri,  4 Nov 2022 12:57:25 -0700
> Anthony DeRossi <ajderossi@gmail.com> wrote:
> 
> > In vfio_device_open(), vfio_container_device_register() is always called
> > when open_count == 1. On error, vfio_device_container_unregister() is
> > only called when open_count == 1 and close_device is set. This leaks a
> > registration for devices without a close_device implementation.
> > 
> > In vfio_device_fops_release(), vfio_device_container_unregister() is
> > called unconditionally. This can cause a device to be unregistered
> > multiple times.
> > 
> > Treating container device registration/unregistration uniformly (always
> > when open_count == 1) fixes both issues.
> 
> Good catch, I see that Jason does subtly fix this in "vfio: Move
> vfio_device driver open/close code to a function", but I'd rather see
> it more overtly fixed in a discrete patch like this.  All "real"
> drivers provide a close_device callback, but mdpy and mtty do not.

Given it only impacts the samples maybe I should just stick it in the
iommufd series before that patch?

Jason

Re: [PATCH v4 1/3] vfio: Fix container device registration life cycle

[Alex Williamson]

On Tue, 8 Nov 2022 20:51:43 -0400
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Fri, Nov 04, 2022 at 02:59:15PM -0600, Alex Williamson wrote:
> > On Fri,  4 Nov 2022 12:57:25 -0700
> > Anthony DeRossi <ajderossi@gmail.com> wrote:
> >   
> > > In vfio_device_open(), vfio_container_device_register() is always called
> > > when open_count == 1. On error, vfio_device_container_unregister() is
> > > only called when open_count == 1 and close_device is set. This leaks a
> > > registration for devices without a close_device implementation.
> > > 
> > > In vfio_device_fops_release(), vfio_device_container_unregister() is
> > > called unconditionally. This can cause a device to be unregistered
> > > multiple times.
> > > 
> > > Treating container device registration/unregistration uniformly (always
> > > when open_count == 1) fixes both issues.  
> > 
> > Good catch, I see that Jason does subtly fix this in "vfio: Move
> > vfio_device driver open/close code to a function", but I'd rather see
> > it more overtly fixed in a discrete patch like this.  All "real"
> > drivers provide a close_device callback, but mdpy and mtty do not.  
> 
> Given it only impacts the samples maybe I should just stick it in the
> iommufd series before that patch?

The series in general though fixes a regression.  Is there any reason
we shouldn't try to push it into 6.1?  Thanks,

Alex

Re: [PATCH v4 1/3] vfio: Fix container device registration life cycle

[Jason Gunthorpe]

On Tue, Nov 08, 2022 at 05:58:38PM -0700, Alex Williamson wrote:
> On Tue, 8 Nov 2022 20:51:43 -0400
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Fri, Nov 04, 2022 at 02:59:15PM -0600, Alex Williamson wrote:
> > > On Fri,  4 Nov 2022 12:57:25 -0700
> > > Anthony DeRossi <ajderossi@gmail.com> wrote:
> > >   
> > > > In vfio_device_open(), vfio_container_device_register() is always called
> > > > when open_count == 1. On error, vfio_device_container_unregister() is
> > > > only called when open_count == 1 and close_device is set. This leaks a
> > > > registration for devices without a close_device implementation.
> > > > 
> > > > In vfio_device_fops_release(), vfio_device_container_unregister() is
> > > > called unconditionally. This can cause a device to be unregistered
> > > > multiple times.
> > > > 
> > > > Treating container device registration/unregistration uniformly (always
> > > > when open_count == 1) fixes both issues.  
> > > 
> > > Good catch, I see that Jason does subtly fix this in "vfio: Move
> > > vfio_device driver open/close code to a function", but I'd rather see
> > > it more overtly fixed in a discrete patch like this.  All "real"
> > > drivers provide a close_device callback, but mdpy and mtty do not.  
> > 
> > Given it only impacts the samples maybe I should just stick it in the
> > iommufd series before that patch?
> 
> The series in general though fixes a regression.  Is there any reason
> we shouldn't try to push it into 6.1?  Thanks,

That works for me too.

Jason

[PATCH v4 2/3] vfio: Add an open counter to vfio_device_set

[Anthony DeRossi]

open_count is incremented before open_device() and decremented after
close_device() for each device in the set. This allows devices to
determine whether shared resources are in use without tracking them
manually or accessing the private open_count in vfio_device.

Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
---
 drivers/vfio/vfio_main.c | 3 +++
 include/linux/vfio.h     | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 9a4af880e941..6c65418fc7e3 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -761,6 +761,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
 		mutex_lock(&device->group->group_lock);
 		device->kvm = device->group->kvm;
 
+		device->dev_set->open_count++;
 		if (device->ops->open_device) {
 			ret = device->ops->open_device(device);
 			if (ret)
@@ -809,6 +810,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
 	}
 err_undo_count:
 	mutex_unlock(&device->group->group_lock);
+	device->dev_set->open_count--;
 	device->open_count--;
 	if (device->open_count == 0 && device->kvm)
 		device->kvm = NULL;
@@ -1023,6 +1025,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 			device->ops->close_device(device);
 
 		vfio_device_container_unregister(device);
+		device->dev_set->open_count--;
 	}
 	mutex_unlock(&device->group->group_lock);
 	device->open_count--;
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index e7cebeb875dd..5becdcdf4ba2 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -28,6 +28,7 @@ struct vfio_device_set {
 	struct mutex lock;
 	struct list_head device_list;
 	unsigned int device_count;
+	unsigned int open_count;
 };
 
 struct vfio_device {
-- 
2.37.4

Re: [PATCH v4 2/3] vfio: Add an open counter to vfio_device_set

[Alex Williamson]

On Fri,  4 Nov 2022 12:57:26 -0700
Anthony DeRossi <ajderossi@gmail.com> wrote:

> open_count is incremented before open_device() and decremented after
> close_device() for each device in the set. This allows devices to
> determine whether shared resources are in use without tracking them
> manually or accessing the private open_count in vfio_device.
> 
> Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
> ---
>  drivers/vfio/vfio_main.c | 3 +++
>  include/linux/vfio.h     | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 9a4af880e941..6c65418fc7e3 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -761,6 +761,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  		mutex_lock(&device->group->group_lock);
>  		device->kvm = device->group->kvm;
>  
> +		device->dev_set->open_count++;

Note that this is on the device first open path...

>  		if (device->ops->open_device) {
>  			ret = device->ops->open_device(device);
>  			if (ret)
> @@ -809,6 +810,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  	}
>  err_undo_count:
>  	mutex_unlock(&device->group->group_lock);
> +	device->dev_set->open_count--;

But this can be reached for non-first open faults.

>  	device->open_count--;
>  	if (device->open_count == 0 && device->kvm)
>  		device->kvm = NULL;
> @@ -1023,6 +1025,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  			device->ops->close_device(device);
>  
>  		vfio_device_container_unregister(device);
> +		device->dev_set->open_count--;

This is on the last-close path, so aside from the above bug this is more
of an open device counter across the device set rather than than a
number of open device file descriptors counter as we have on each
device.

There's some complexity thinking about the difference between those for
our scenario, it works, but requires a bit of deduction.  For example,
a device set might have a count of 1 here, but that one device could be
opened multiple times.  It works for the scenario you address in the
next patch, but is maybe not as generically useful.

Like it seems maybe you're going for by your more recent comment, I was
thinking an interface rather than tracking a new field on the device
set.  Thanks,

Alex

>  	}
>  	mutex_unlock(&device->group->group_lock);
>  	device->open_count--;
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index e7cebeb875dd..5becdcdf4ba2 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -28,6 +28,7 @@ struct vfio_device_set {
>  	struct mutex lock;
>  	struct list_head device_list;
>  	unsigned int device_count;
> +	unsigned int open_count;
>  };
>  
>  struct vfio_device {

Re: [PATCH v4 2/3] vfio: Add an open counter to vfio_device_set

[Anthony DeRossi]

On Fri, Nov 4, 2022 at 8:59 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
> Like it seems maybe you're going for by your more recent comment, I was
> thinking an interface rather than tracking a new field on the device
> set.

Thanks for the feedback. I sent an updated series with this change.

v5: https://lore.kernel.org/kvm/20221105224458.8180-1-ajderossi@gmail.com/

Anthony

[PATCH v4 3/3] vfio/pci: Check the device set open_count on reset

[Anthony DeRossi]

vfio_pci_dev_set_needs_reset() inspects the open_count of every device
in the set to determine whether a reset is needed. The current device
always has open_count == 1 within vfio_pci_core_disable(), effectively
disabling the reset logic. This field is also documented as private in
vfio_device, so it should not be used to determine whether other devices
in the set are open.

Checking for open_count > 1 on the device set fixes both issues.

After commit 2cd8b14aaa66 ("vfio/pci: Move to the device set
infrastructure"), failure to create a new file for a device would cause
the reset to be skipped due to open_count being decremented after
calling close_device() in the error path.

After commit eadd86f835c6 ("vfio: Remove calls to
vfio_group_add_container_user()"), releasing a device would always skip
the reset due to an ordering change in vfio_device_fops_release().

Failing to reset the device leaves it in an unknown state, potentially
causing errors when it is bound to a different driver.

This issue was observed with a Radeon RX Vega 56 [1002:687f] (rev c3)
assigned to a Windows guest. After shutting down the guest, unbinding
the device from vfio-pci, and binding the device to amdgpu:

[  548.007102] [drm:psp_hw_start [amdgpu]] *ERROR* PSP create ring failed!
[  548.027174] [drm:psp_hw_init [amdgpu]] *ERROR* PSP firmware loading failed
[  548.027242] [drm:amdgpu_device_fw_loading [amdgpu]] *ERROR* hw_init of IP block <psp> failed -22
[  548.027306] amdgpu 0000:0a:00.0: amdgpu: amdgpu_device_ip_init failed
[  548.027308] amdgpu 0000:0a:00.0: amdgpu: Fatal error during GPU init

Fixes: 2cd8b14aaa66 ("vfio/pci: Move to the device set infrastructure")
Fixes: eadd86f835c6 ("vfio: Remove calls to vfio_group_add_container_user()")
Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index badc9d828cac..e65c70781fe2 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2488,12 +2488,12 @@ static bool vfio_pci_dev_set_needs_reset(struct vfio_device_set *dev_set)
 	struct vfio_pci_core_device *cur;
 	bool needs_reset = false;
 
-	list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) {
-		/* No VFIO device in the set can have an open device FD */
-		if (cur->vdev.open_count)
-			return false;
+	/* No other VFIO device in the set can be open. */
+	if (dev_set->open_count > 1)
+		return false;
+
+	list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list)
 		needs_reset |= cur->needs_reset;
-	}
 	return needs_reset;
 }
 
-- 
2.37.4