| Message ID | 20221104195727.4629-2-ajderossi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vfio/pci: Check the device set open_count on reset | expand |
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)
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
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
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
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)
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(-)