On Fri, 1 Oct 2021 20:22:22 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
> --- a/drivers/vfio/vfio.c
> +++ 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, but in any case we're freeing the
object that we return? That can't be right.
> return group;
> }
>
> @@ -439,7 +445,6 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
> static void vfio_group_release(struct kref *kref)
> {
> struct vfio_group *group = container_of(kref, struct vfio_group, kref);
> - struct iommu_group *iommu_group = group->iommu_group;
>
> WARN_ON(!list_empty(&group->device_list));
> WARN_ON(atomic_read(&group->container_users));
> @@ -449,7 +454,6 @@ static void vfio_group_release(struct kref *kref)
> list_del(&group->vfio_next);
> vfio_free_group_minor(group->minor);
> vfio_group_unlock_and_free(group);
> - iommu_group_put(iommu_group);
> }
>
> static void vfio_group_put(struct vfio_group *group)
> @@ -734,7 +738,7 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
> ret = PTR_ERR(group);
> goto out_remove_device;
> }
> -
> + iommu_group_put(iommu_group);
> return group;
>
> out_remove_device:
> @@ -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,
Alex
> group = vfio_create_group(iommu_group, VFIO_IOMMU);
> - if (IS_ERR(group))
> - goto out_put;
> - return group;
> -
> out_put:
> iommu_group_put(iommu_group);
> return group;
