Re: [PATCH 1/5] vfio: Delete vfio_get/put_group from vfio_iommu_group_notifier()

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Oct 04, 2021 at 04:25:32PM -0600, Alex Williamson wrote:
> On Fri,  1 Oct 2021 20:22:20 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > iommu_group_register_notifier()/iommu_group_unregister_notifier() are
> > built using a blocking_notifier_chain which integrates a rwsem. The
> > notifier function cannot be running outside its registration.
> > 
> > When considering how the notifier function interacts with create/destroy
> > of the group there are two fringe cases, the notifier starts before
> > list_add(&vfio.group_list) and the notifier runs after the kref
> > becomes 0.
> > 
> > Prior to vfio_create_group() unlocking and returning we have
> >    container_users == 0
> >    device_list == empty
> > And this cannot change until the mutex is unlocked.
> > 
> > After the kref goes to zero we must also have
> >    container_users == 0
> >    device_list == empty
> > 
> > Both are required because they are balanced operations and a 0 kref means
> > some caller became unbalanced. Add the missing assertion that
> > container_users must be zero as well.
> > 
> > These two facts are important because when checking each operation we see:
> > 
> > - IOMMU_GROUP_NOTIFY_ADD_DEVICE
> >    Empty device_list avoids the WARN_ON in vfio_group_nb_add_dev()
> >    0 container_users ends the call
> > - IOMMU_GROUP_NOTIFY_BOUND_DRIVER
> >    0 container_users ends the call
> > 
> > Finally, we have IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER, which only deletes
> > items from the unbound list. During creation this list is empty, during
> > kref == 0 nothing can read this list, and it will be freed soon.
> > 
> > Since the vfio_group_release() doesn't hold the appropriate lock to
> > manipulate the unbound_list and could race with the notifier, move the
> > cleanup to directly before the kfree.
> > 
> > This allows deleting all of the deferred group put code.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/vfio.c | 89 +++++----------------------------------------
> >  1 file changed, 9 insertions(+), 80 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 08b27b64f0f935..32a53cb3598524 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -324,12 +324,20 @@ static void vfio_container_put(struct vfio_container *container)
> >  
> >  static void vfio_group_unlock_and_free(struct vfio_group *group)
> >  {
> > +	struct vfio_unbound_dev *unbound, *tmp;
> > +
> >  	mutex_unlock(&vfio.group_lock);
> >  	/*
> >  	 * Unregister outside of lock.  A spurious callback is harmless now
> >  	 * that the group is no longer in vfio.group_list.
> >  	 */
> 
> This comment is indirectly referencing the vfio_group_try_get() in the
> notifier callback, but as you describe in the commit log, it's actually
> the container_users value that prevents this from racing group release
> now.  Otherwise, tricky but looks good.  Thanks,

Do you think the comment should be deleted in this commit? I think I
got it later on..

Jason