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

[Alex Williamson <alex.williamson@redhat.com>]

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.

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

> >  
> >  	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);
> >