[Patch 0/1] A bug fix to the error handling path in vfio_device_open()

[Patch 0/1] A bug fix to the error handling path in vfio_device_open()

[Yi Liu]

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

[Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Yi Liu]

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

Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Matthew Rosato]

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:

Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Yi Liu]

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

RE: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Tian, Kevin]

> 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.

Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Yi Liu]

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

RE: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Tian, Kevin]

> 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.

Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Yi Liu]

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

Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Jason Gunthorpe]

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

Re: [Patch 1/1] vfio: Move "device->open_count--" out of group_rwsem in vfio_device_open()

[Yi Liu]

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