[PATCH] vfio: Decrement open_count before close_device()

[PATCH] vfio: Decrement open_count before close_device()

[Anthony DeRossi]

The implementation of close_device() for vfio-pci inspects the
open_count of every device in the device set to determine whether a
reset is needed. Unless open_count is decremented before invoking
close_device(), the device set will always contain a device with
open_count > 0, effectively disabling the reset logic.

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 when closing the 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.

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/vfio_main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 2d168793d4e1..7c3f1734fb35 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -763,8 +763,10 @@ static struct file *vfio_device_open(struct vfio_device *device)
 
 		if (device->ops->open_device) {
 			ret = device->ops->open_device(device);
-			if (ret)
-				goto err_undo_count;
+			if (ret) {
+				device->open_count--;
+				goto err_unlock;
+			}
 		}
 		vfio_device_container_register(device);
 		mutex_unlock(&device->group->group_lock);
@@ -801,14 +803,13 @@ 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) {
+	if (!--device->open_count && device->ops->close_device) {
 		device->ops->close_device(device);
 
 		vfio_device_container_unregister(device);
 	}
-err_undo_count:
+err_unlock:
 	mutex_unlock(&device->group->group_lock);
-	device->open_count--;
 	if (device->open_count == 0 && device->kvm)
 		device->kvm = NULL;
 	mutex_unlock(&device->dev_set->lock);
@@ -1017,12 +1018,11 @@ 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)
+	if (!--device->open_count && device->ops->close_device)
 		device->ops->close_device(device);
 
 	vfio_device_container_unregister(device);
 	mutex_unlock(&device->group->group_lock);
-	device->open_count--;
 	if (device->open_count == 0)
 		device->kvm = NULL;
 	mutex_unlock(&device->dev_set->lock);
-- 
2.37.4

Re: [PATCH] vfio: Decrement open_count before close_device()

[Jason Gunthorpe]

On Tue, Oct 25, 2022 at 12:38:20PM -0700, Anthony DeRossi wrote:
> The implementation of close_device() for vfio-pci inspects the
> open_count of every device in the device set to determine whether a
> reset is needed. Unless open_count is decremented before invoking
> close_device(), the device set will always contain a device with
> open_count > 0, effectively disabling the reset logic.

This seems to miss the reason why this was done:

    Eliminate the calls to vfio_group_add_container_user() and add
    vfio_assert_device_open() to detect driver mis-use. This causes the
    close_device() op to check device->open_count so always leave it elevated
    while calling the op.

If we let it be zero then vfio_assert_device_open() will trigger on
other drivers.

I think the best approach is to change vfio_pci to understand that
open_count == 1 means it is the last close.

Thanks,
Jason

Re: [PATCH] vfio: Decrement open_count before close_device()

[Anthony DeRossi]

On Wed, Oct 26, 2022 at 11:24 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> I think the best approach is to change vfio_pci to understand that
> open_count == 1 means it is the last close.

Thanks for the feedback. I sent an updated patch.

v2: https://lore.kernel.org/kvm/20221026194245.1769-1-ajderossi@gmail.com/

Anthony