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

[Alexey Kardashevskiy]

On 20/12/16 03:28, Alex Williamson wrote:
> On Mon, 19 Dec 2016 18:41:05 +0800
> Jike Song  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 
>>> ---
>>>
>>> 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(>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,
>>>  >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(>opened);
>>>   
> 


-- 
Alexey

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

[Jike Song]

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

Hi Alexey,

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()?

--
Thanks,
Jike

> ---
>  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(>device_list));
> + /* Any user didn't unregister? */
> + WARN_ON(group->notifier.head);
>  
>   list_for_each_entry_safe(unbound, tmp,
>>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(>opened);
> 

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

[Alex Williamson]

On Mon, 19 Dec 2016 18:41:05 +0800
Jike Song  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 
> > ---
> > 
> > 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.

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

> >  
> > list_for_each_entry_safe(unbound, tmp,
> >  >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(>opened);
> >