* [Patch 0/1] A bug fix to the error handling path in vfio_device_open()
@ 2022-06-20 8:54 Yi Liu
2022-06-20 8:54 ` [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem " Yi Liu
0 siblings, 1 reply; 10+ messages in thread
From: Yi Liu @ 2022-06-20 8:54 UTC (permalink / raw)
To: alex.williamson; +Cc: yi.l.liu, kevin.tian, kvm, jgg
The open_count is unnecessarily placed in the group_rwsem in the error
handling path.
Yi Liu (1):
vfio: Move "device->open_count--" out of group_rwsem in
vfio_device_open()
drivers/vfio/vfio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-20 8:54 [Patch 0/1] A bug fix to the error handling path in vfio_device_open() Yi Liu
@ 2022-06-20 8:54 ` Yi Liu
2022-06-20 20:13 ` Matthew Rosato
2022-06-24 14:05 ` Jason Gunthorpe
0 siblings, 2 replies; 10+ messages in thread
From: Yi Liu @ 2022-06-20 8:54 UTC (permalink / raw)
To: alex.williamson; +Cc: yi.l.liu, kevin.tian, kvm, jgg, Matthew Rosato
No need to protect open_count with group_rwsem
Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
cc: Matthew Rosato <mjrosato@linux.ibm.com>
cc: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
drivers/vfio/vfio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 61e71c1154be..44c3bf8023ac 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1146,10 +1146,10 @@ static struct file *vfio_device_open(struct vfio_device *device)
if (device->open_count == 1 && device->ops->close_device)
device->ops->close_device(device);
err_undo_count:
+ up_read(&device->group->group_rwsem);
device->open_count--;
if (device->open_count == 0 && device->kvm)
device->kvm = NULL;
- up_read(&device->group->group_rwsem);
mutex_unlock(&device->dev_set->lock);
module_put(device->dev->driver->owner);
err_unassign_container:
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-20 8:54 ` [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem " Yi Liu
@ 2022-06-20 20:13 ` Matthew Rosato
2022-06-21 1:31 ` Yi Liu
2022-06-24 14:05 ` Jason Gunthorpe
1 sibling, 1 reply; 10+ messages in thread
From: Matthew Rosato @ 2022-06-20 20:13 UTC (permalink / raw)
To: Yi Liu, alex.williamson; +Cc: kevin.tian, kvm, jgg
On 6/20/22 4:54 AM, Yi Liu wrote:
> No need to protect open_count with group_rwsem
>
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>
> cc: Matthew Rosato <mjrosato@linux.ibm.com>
> cc: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Seems pretty harmless as-is, but you are correct group_rwsem can be
dropped earlier; we do not protect the count with group_rwsem elsewhere
(see vfio_device_fops_release as a comparison, where we already drop
group_rwsem before open_count--)
FWIW, this change now also drops group_rswem before setting device->kvm
= NULL, but that's also OK (again, just like vfio_device_fops_release)
-- While the setting of device->kvm before open_device is technically
done while holding the group_rwsem, this is done to protect the group
kvm value we are copying from, and we should not be relying on that to
protect the contents of device->kvm; instead we assume this value will
not change until after the device is closed and while under the
dev_set->lock.
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
> drivers/vfio/vfio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 61e71c1154be..44c3bf8023ac 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1146,10 +1146,10 @@ static struct file *vfio_device_open(struct vfio_device *device)
> if (device->open_count == 1 && device->ops->close_device)
> device->ops->close_device(device);
> err_undo_count:
> + up_read(&device->group->group_rwsem);
> device->open_count--;
> if (device->open_count == 0 && device->kvm)
> device->kvm = NULL;
> - up_read(&device->group->group_rwsem);
> mutex_unlock(&device->dev_set->lock);
> module_put(device->dev->driver->owner);
> err_unassign_container:
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-20 20:13 ` Matthew Rosato
@ 2022-06-21 1:31 ` Yi Liu
2022-06-21 2:49 ` Tian, Kevin
0 siblings, 1 reply; 10+ messages in thread
From: Yi Liu @ 2022-06-21 1:31 UTC (permalink / raw)
To: Matthew Rosato, alex.williamson; +Cc: kevin.tian, kvm, jgg
On 2022/6/21 04:13, Matthew Rosato wrote:
> On 6/20/22 4:54 AM, Yi Liu wrote:
>> No need to protect open_count with group_rwsem
>>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>>
>> cc: Matthew Rosato <mjrosato@linux.ibm.com>
>> cc: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>
> Seems pretty harmless as-is, but you are correct group_rwsem can be dropped
> earlier; we do not protect the count with group_rwsem elsewhere (see
> vfio_device_fops_release as a comparison, where we already drop group_rwsem
> before open_count--)
yes. this is exactly how I found it. Normally, I compare the err handling
path with the release function to see if they are aligned. :-)
> FWIW, this change now also drops group_rswem before setting device->kvm =
> NULL, but that's also OK (again, just like vfio_device_fops_release) --
> While the setting of device->kvm before open_device is technically done
> while holding the group_rwsem, this is done to protect the group kvm value
> we are copying from, and we should not be relying on that to protect the
> contents of device->kvm; instead we assume this value will not change until
> after the device is closed and while under the dev_set->lock.
yes. set device->kvm to be NULL has no need to hold group_rwsem. BTW. I
also doubt whether the device->ops->open_device(device) and
device->ops->close_device(device) should be protected by group_rwsem or
not. seems not, right? group_rwsem protects the fields under vfio_group.
For the open_device/close_device() device->dev_set->lock is enough. Maybe
another nit fix.
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
>> ---
>> drivers/vfio/vfio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 61e71c1154be..44c3bf8023ac 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1146,10 +1146,10 @@ static struct file *vfio_device_open(struct
>> vfio_device *device)
>> if (device->open_count == 1 && device->ops->close_device)
>> device->ops->close_device(device);
>> err_undo_count:
>> + up_read(&device->group->group_rwsem);
>> device->open_count--;
>> if (device->open_count == 0 && device->kvm)
>> device->kvm = NULL;
>> - up_read(&device->group->group_rwsem);
>> mutex_unlock(&device->dev_set->lock);
>> module_put(device->dev->driver->owner);
>> err_unassign_container:
>
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-21 1:31 ` Yi Liu
@ 2022-06-21 2:49 ` Tian, Kevin
2022-06-21 2:59 ` Yi Liu
0 siblings, 1 reply; 10+ messages in thread
From: Tian, Kevin @ 2022-06-21 2:49 UTC (permalink / raw)
To: Liu, Yi L, Matthew Rosato, alex.williamson; +Cc: kvm, jgg
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, June 21, 2022 9:32 AM
>
> On 2022/6/21 04:13, Matthew Rosato wrote:
> > On 6/20/22 4:54 AM, Yi Liu wrote:
> >> No need to protect open_count with group_rwsem
> >>
> >> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
> >>
> >> cc: Matthew Rosato <mjrosato@linux.ibm.com>
> >> cc: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> >
> > Seems pretty harmless as-is, but you are correct group_rwsem can be
> dropped
> > earlier; we do not protect the count with group_rwsem elsewhere (see
> > vfio_device_fops_release as a comparison, where we already drop
> group_rwsem
> > before open_count--)
>
> yes. this is exactly how I found it. Normally, I compare the err handling
> path with the release function to see if they are aligned. :-)
In this case we don't need a FIX tag. It's kind of optimization.
>
> > FWIW, this change now also drops group_rswem before setting device-
> >kvm =
> > NULL, but that's also OK (again, just like vfio_device_fops_release) --
> > While the setting of device->kvm before open_device is technically done
> > while holding the group_rwsem, this is done to protect the group kvm
> value
> > we are copying from, and we should not be relying on that to protect the
> > contents of device->kvm; instead we assume this value will not change until
> > after the device is closed and while under the dev_set->lock.
>
> yes. set device->kvm to be NULL has no need to hold group_rwsem. BTW. I
> also doubt whether the device->ops->open_device(device) and
> device->ops->close_device(device) should be protected by group_rwsem or
> not. seems not, right? group_rwsem protects the fields under vfio_group.
> For the open_device/close_device() device->dev_set->lock is enough. Maybe
> another nit fix.
>
group->rwsem is to protect device->group->kvm from being changed
by vfio_file_set_kvm() before it is copied to device->kvm.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-21 2:49 ` Tian, Kevin
@ 2022-06-21 2:59 ` Yi Liu
2022-06-21 3:26 ` Tian, Kevin
0 siblings, 1 reply; 10+ messages in thread
From: Yi Liu @ 2022-06-21 2:59 UTC (permalink / raw)
To: Tian, Kevin, Matthew Rosato, alex.williamson; +Cc: kvm, jgg
On 2022/6/21 10:49, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, June 21, 2022 9:32 AM
>>
>> On 2022/6/21 04:13, Matthew Rosato wrote:
>>> On 6/20/22 4:54 AM, Yi Liu wrote:
>>>> No need to protect open_count with group_rwsem
>>>>
>>>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>>>>
>>>> cc: Matthew Rosato <mjrosato@linux.ibm.com>
>>>> cc: Jason Gunthorpe <jgg@nvidia.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>
>>> Seems pretty harmless as-is, but you are correct group_rwsem can be
>> dropped
>>> earlier; we do not protect the count with group_rwsem elsewhere (see
>>> vfio_device_fops_release as a comparison, where we already drop
>> group_rwsem
>>> before open_count--)
>>
>> yes. this is exactly how I found it. Normally, I compare the err handling
>> path with the release function to see if they are aligned. :-)
>
> In this case we don't need a FIX tag. It's kind of optimization.
ok.
>>
>>> FWIW, this change now also drops group_rswem before setting device-
>>> kvm =
>>> NULL, but that's also OK (again, just like vfio_device_fops_release) --
>>> While the setting of device->kvm before open_device is technically done
>>> while holding the group_rwsem, this is done to protect the group kvm
>> value
>>> we are copying from, and we should not be relying on that to protect the
>>> contents of device->kvm; instead we assume this value will not change until
>>> after the device is closed and while under the dev_set->lock.
>>
>> yes. set device->kvm to be NULL has no need to hold group_rwsem. BTW. I
>> also doubt whether the device->ops->open_device(device) and
>> device->ops->close_device(device) should be protected by group_rwsem or
>> not. seems not, right? group_rwsem protects the fields under vfio_group.
>> For the open_device/close_device() device->dev_set->lock is enough. Maybe
>> another nit fix.
>>
>
> group->rwsem is to protect device->group->kvm from being changed
> by vfio_file_set_kvm() before it is copied to device->kvm.
yes. this is why vfio_device_open() holds the read lock of group_rwsem
around the device->group->kvm copy. However, for the open_device(),
callback, I don't think it is necessary to be protected by the group_rwsem
lock.
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-21 2:59 ` Yi Liu
@ 2022-06-21 3:26 ` Tian, Kevin
2022-06-21 3:35 ` Yi Liu
0 siblings, 1 reply; 10+ messages in thread
From: Tian, Kevin @ 2022-06-21 3:26 UTC (permalink / raw)
To: Liu, Yi L, Matthew Rosato, alex.williamson; +Cc: kvm, jgg
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, June 21, 2022 10:59 AM
> >>
> >>> FWIW, this change now also drops group_rswem before setting device-
> >>> kvm =
> >>> NULL, but that's also OK (again, just like vfio_device_fops_release) --
> >>> While the setting of device->kvm before open_device is technically done
> >>> while holding the group_rwsem, this is done to protect the group kvm
> >> value
> >>> we are copying from, and we should not be relying on that to protect the
> >>> contents of device->kvm; instead we assume this value will not change
> until
> >>> after the device is closed and while under the dev_set->lock.
> >>
> >> yes. set device->kvm to be NULL has no need to hold group_rwsem. BTW.
> I
> >> also doubt whether the device->ops->open_device(device) and
> >> device->ops->close_device(device) should be protected by group_rwsem
> or
> >> not. seems not, right? group_rwsem protects the fields under vfio_group.
> >> For the open_device/close_device() device->dev_set->lock is enough.
> Maybe
> >> another nit fix.
> >>
> >
> > group->rwsem is to protect device->group->kvm from being changed
> > by vfio_file_set_kvm() before it is copied to device->kvm.
>
> yes. this is why vfio_device_open() holds the read lock of group_rwsem
> around the device->group->kvm copy. However, for the open_device(),
> callback, I don't think it is necessary to be protected by the group_rwsem
> lock.
>
The kvm object could be freed after device->kvm is set, if
group_rwsem is not held when calling open_device(). Then you'll
hit another use-after-free bug when mdev driver tries to obtain
a reference on kvm.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-21 3:26 ` Tian, Kevin
@ 2022-06-21 3:35 ` Yi Liu
0 siblings, 0 replies; 10+ messages in thread
From: Yi Liu @ 2022-06-21 3:35 UTC (permalink / raw)
To: Tian, Kevin, Matthew Rosato, alex.williamson; +Cc: kvm, jgg
On 2022/6/21 11:26, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, June 21, 2022 10:59 AM
>>>>
>>>>> FWIW, this change now also drops group_rswem before setting device-
>>>>> kvm =
>>>>> NULL, but that's also OK (again, just like vfio_device_fops_release) --
>>>>> While the setting of device->kvm before open_device is technically done
>>>>> while holding the group_rwsem, this is done to protect the group kvm
>>>> value
>>>>> we are copying from, and we should not be relying on that to protect the
>>>>> contents of device->kvm; instead we assume this value will not change
>> until
>>>>> after the device is closed and while under the dev_set->lock.
>>>>
>>>> yes. set device->kvm to be NULL has no need to hold group_rwsem. BTW.
>> I
>>>> also doubt whether the device->ops->open_device(device) and
>>>> device->ops->close_device(device) should be protected by group_rwsem
>> or
>>>> not. seems not, right? group_rwsem protects the fields under vfio_group.
>>>> For the open_device/close_device() device->dev_set->lock is enough.
>> Maybe
>>>> another nit fix.
>>>>
>>>
>>> group->rwsem is to protect device->group->kvm from being changed
>>> by vfio_file_set_kvm() before it is copied to device->kvm.
>>
>> yes. this is why vfio_device_open() holds the read lock of group_rwsem
>> around the device->group->kvm copy. However, for the open_device(),
>> callback, I don't think it is necessary to be protected by the group_rwsem
>> lock.
>>
>
> The kvm object could be freed after device->kvm is set, if
> group_rwsem is not held when calling open_device(). Then you'll
> hit another use-after-free bug when mdev driver tries to obtain
> a reference on kvm.
aha. I see. so group_rwsem prevents the kvm object free as such a thread
should be blocked in the vfio_file_set_kvm() if the vfio_device_open() has
held the group_rwsem. Hence kvm object is safe to be used. thanks!
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-20 8:54 ` [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem " Yi Liu
2022-06-20 20:13 ` Matthew Rosato
@ 2022-06-24 14:05 ` Jason Gunthorpe
2022-06-27 7:45 ` Yi Liu
1 sibling, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2022-06-24 14:05 UTC (permalink / raw)
To: Yi Liu; +Cc: alex.williamson, kevin.tian, kvm, Matthew Rosato
On Mon, Jun 20, 2022 at 01:54:59AM -0700, Yi Liu wrote:
> No need to protect open_count with group_rwsem
You should explain why this is a good change as you did in the emails
here, and no cover letter required for single patches
> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>
Extra space after fixes line.
No Fixes line for things that are not bugs.
> cc: Matthew Rosato <mjrosato@linux.ibm.com>
> cc: Jason Gunthorpe <jgg@nvidia.com>
Cc:
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> drivers/vfio/vfio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
It seems OK but a bit unnecessary.
Jason
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()
2022-06-24 14:05 ` Jason Gunthorpe
@ 2022-06-27 7:45 ` Yi Liu
0 siblings, 0 replies; 10+ messages in thread
From: Yi Liu @ 2022-06-27 7:45 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: alex.williamson, kevin.tian, kvm, Matthew Rosato
On 2022/6/24 22:05, Jason Gunthorpe wrote:
> On Mon, Jun 20, 2022 at 01:54:59AM -0700, Yi Liu wrote:
>> No need to protect open_count with group_rwsem
>
> You should explain why this is a good change as you did in the emails
> here, and no cover letter required for single patches
>
>> Fixes: 421cfe6596f6 ("vfio: remove VFIO_GROUP_NOTIFY_SET_KVM")
>>
>
> Extra space after fixes line.
> No Fixes line for things that are not bugs.
>
>> cc: Matthew Rosato <mjrosato@linux.ibm.com>
>> cc: Jason Gunthorpe <jgg@nvidia.com>
>
> Cc:
>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>> drivers/vfio/vfio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> It seems OK but a bit unnecessary.
>
thanks, Jason. all good suggestions.I'd just want to see the
device->dev_set->lock and the group_rwsem hold/release is
symmetric in the case of err_undo_count is hit in the vfio_device_open().
Just like the order in vfio_device_fops_release().
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-06-27 7:45 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-20 8:54 [Patch 0/1] A bug fix to the error handling path in vfio_device_open() Yi Liu
2022-06-20 8:54 ` [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem " Yi Liu
2022-06-20 20:13 ` Matthew Rosato
2022-06-21 1:31 ` Yi Liu
2022-06-21 2:49 ` Tian, Kevin
2022-06-21 2:59 ` Yi Liu
2022-06-21 3:26 ` Tian, Kevin
2022-06-21 3:35 ` Yi Liu
2022-06-24 14:05 ` Jason Gunthorpe
2022-06-27 7:45 ` Yi Liu
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.