Re: [PATCH kernel v2 10/11] vfio: Check for unregistered notifiers when group is actually released

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 20/12/16 03:28, Alex Williamson wrote:
> On Mon, 19 Dec 2016 18:41:05 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 12/18/2016 09:28 AM, Alexey Kardashevskiy wrote:
>>> This moves a check for unregistered notifiers from fops release
>>> callback to the place where the group will actually be released.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>
>>> This is going to be used in the following patch in cleanup
>>> path. Since the next patch is RFC, this one might not be needed.  
> 
> Alexey, this is intended to be a bug fix, it should be sent separately,
> not buried in an unrelated patch series.

Well, I was pretty unsure this is a correct fix and I was sure that it
would make sense without the context which is quite weird :)

> 
>> I didn't find any use in the following patch 11/11, did you mean
>> something else?
>>
>> BTW the warning in vfio_group_release seems too late, the user
>> should actually unregister everything by close()?
> 
> The thing is, it's not the user that registered the notifiers, it's the
> vendor driver.  The vendor driver should know via the device release to
> unregister the notifier, which we're counting on to happen before the
> group release.  Can we rely on that ordering even in the case where a
> user is SIGKILL'd?
> 
>>> ---
>>>  drivers/vfio/vfio.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index 6b9a98508939..083b581e87c0 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -403,6 +403,8 @@ static void vfio_group_release(struct kref *kref)
>>>  	struct iommu_group *iommu_group = group->iommu_group;
>>>  
>>>  	WARN_ON(!list_empty(&group->device_list));
>>> +	/* Any user didn't unregister? */
>>> +	WARN_ON(group->notifier.head);
> 
> Even if this is a bug, this is the wrong fix.  This is when the group
> is being destroyed.  Yes, it would be a bug to still have any notifiers
> here, but the intention is to make sure there are no notifiers when the
> group is idle and unused.

Out of curiosity - vendor drivers are supposed to hold a group file open,
not just reference vfio_grop/iommu_group objects?



> 
>>>  
>>>  	list_for_each_entry_safe(unbound, tmp,
>>>  				 &group->unbound_list, unbound_next) {
>>> @@ -1584,9 +1586,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
>>>  
>>>  	filep->private_data = NULL;
>>>  
>>> -	/* Any user didn't unregister? */
>>> -	WARN_ON(group->notifier.head);
>>> -
>>>  	vfio_group_try_dissolve_container(group);
>>>  
>>>  	atomic_dec(&group->opened);
>>>   
> 


-- 
Alexey