Re: [PATCH 3/5] vfio: Don't leak a group reference if the group already exists

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

On Mon, 4 Oct 2021 19:36:41 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Oct 04, 2021 at 04:25:43PM -0600, Alex Williamson wrote:
> > On Fri,  1 Oct 2021 20:22:22 -0300
> > Jason Gunthorpe <jgg@nvidia.com> wrote:
> >   
> > > If vfio_create_group() searches the group list and returns an already
> > > existing group it does not put back the iommu_group reference that the
> > > caller passed in.
> > > 
> > > Change the semantic of vfio_create_group() to not move the reference in
> > > from the caller, but instead obtain a new reference inside and leave the
> > > caller's reference alone. The two callers must now call iommu_group_put().
> > > 
> > > This is an unlikely race as the only caller that could hit it has already
> > > searched the group list before attempting to create the group.
> > > 
> > > Fixes: cba3345cc494 ("vfio: VFIO core")
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  drivers/vfio/vfio.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > > index 1cb12033b02240..bf233943dc992f 100644
> > > +++ b/drivers/vfio/vfio.c
> > > @@ -338,6 +338,7 @@ static void vfio_group_unlock_and_free(struct vfio_group *group)
> > >  		list_del(&unbound->unbound_next);
> > >  		kfree(unbound);
> > >  	}
> > > +	iommu_group_put(group->iommu_group);
> > >  	kfree(group);
> > >  }
> > >  
> > > @@ -389,6 +390,8 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
> > >  	atomic_set(&group->opened, 0);
> > >  	init_waitqueue_head(&group->container_q);
> > >  	group->iommu_group = iommu_group;
> > > +	/* put in vfio_group_unlock_and_free() */
> > > +	iommu_group_ref_get(iommu_group);  
> 
>       ^^^^^^^^^^^^^^^^^
> 
> > >  	group->type = type;
> > >  	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
> > >  
> > > @@ -396,8 +399,8 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
> > >  
> > >  	ret = iommu_group_register_notifier(iommu_group, &group->nb);
> > >  	if (ret) {
> > > -		kfree(group);
> > > -		return ERR_PTR(ret);
> > > +		group = ERR_PTR(ret);
> > > +		goto err_put_group;
> > >  	}
> > >  
> > >  	mutex_lock(&vfio.group_lock);
> > > @@ -432,6 +435,9 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
> > >  
> > >  	mutex_unlock(&vfio.group_lock);
> > >  
> > > +err_put_group:
> > > +	iommu_group_put(iommu_group);
> > > +	kfree(group);  
> > 
> > ????
> > 
> > In the non-error path we're releasing the caller's reference which is
> > now their responsibility to release,  
> 
> This release is paried with the get in the same function added one
> hunk above

Note that this is the common exit path until the last patch in the
series pulls returning the successfully created/found group above the
error condition exit paths.  As it stands, this patch unconditionally
releases the reference it claims to newly create.  Thanks,

Alex
 
> > but in any case we're freeing the object that we return?  That
> > can't be right.  
> 
> Yes, that is a rebasing mistake pulling this back from the last patch
> that had a "return ret" here, thanks
> 
> > > @@ -776,10 +780,6 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
> > >  
> > >  	/* a newly created vfio_group keeps the reference. */  
> > 
> > This comment is now incorrect.  Thanks,  
> 
> Indeed
> 
> Jason
>