* [PATCH] vfio: Decrement open_count before close_device()
@ 2022-10-25 19:38 Anthony DeRossi
2022-10-26 11:24 ` Jason Gunthorpe
0 siblings, 1 reply; 3+ messages in thread
From: Anthony DeRossi @ 2022-10-25 19:38 UTC (permalink / raw)
To: kvm; +Cc: alex.williamson, cohuck, jgg, yishaih, kevin.tian
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] vfio: Decrement open_count before close_device()
2022-10-25 19:38 [PATCH] vfio: Decrement open_count before close_device() Anthony DeRossi
@ 2022-10-26 11:24 ` Jason Gunthorpe
2022-10-26 19:57 ` Anthony DeRossi
0 siblings, 1 reply; 3+ messages in thread
From: Jason Gunthorpe @ 2022-10-26 11:24 UTC (permalink / raw)
To: Anthony DeRossi; +Cc: kvm, alex.williamson, cohuck, yishaih, kevin.tian
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-26 19:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-25 19:38 [PATCH] vfio: Decrement open_count before close_device() Anthony DeRossi
2022-10-26 11:24 ` Jason Gunthorpe
2022-10-26 19:57 ` Anthony DeRossi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).