[PATCH v2] drm/vmwgfx: Fix dumb buffer leak

[Zack Rusin]

On Thu, Jan 23, 2025 at 3:44 PM Ian Forbes <ian.forbes at broadcom.com> wrote:
>
> Dumb buffers were not being freed because the GEM reference that was
> acquired in gb_surface_define was not dropped like it is in the 2D case.
> Dropping this ref uncovered a few additional issues with freeing the
> resources associated with dirty tracking in vmw_bo_free/release.
>
> Additionally the TTM object associated with the surface were also leaking
> which meant that when the ttm_object_file was closed at process exit the
> destructor unreferenced an already destroyed surface.
>
> The solution is to remove the destructor from the vmw_user_surface
> associated with the dumb_buffer and immediately unreferencing the TTM
> object which his removes it from the ttm_object_file.
>
> This also allows the early return in vmw_user_surface_base_release for the
> dumb buffer case to be removed as it should no longer occur.
>
> The chain of references now has the GEM handle(s) owning the dumb buffer.
> The GEM handles have a singular GEM reference to the vmw_bo which is
> dropped when all handles are closed. When the GEM reference count hits
> zero the vmw_bo is freed which then unreferences the surface via
> vmw_resource_release in vmw_bo_release.
>
> Fixes: d6667f0ddf46 ("drm/vmwgfx: Fix handling of dumb buffers")
> Signed-off-by: Ian Forbes <ian.forbes at broadcom.com>
> ---
>
> v2:
>  - Update commit description
>  - Clean up leaked dirty tracking resources
>  - Fix handling of leaked TTM objects
>
>  drivers/gpu/drm/vmwgfx/vmwgfx_bo.c       |  6 ++++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c |  2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c  | 18 ++++++++++++------
>  3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 9b5b8c1f063b..87d8b2ba0e8c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -51,11 +51,13 @@ static void vmw_bo_release(struct vmw_bo *vbo)
>                         mutex_lock(&res->dev_priv->cmdbuf_mutex);
>                         (void)vmw_resource_reserve(res, false, true);
>                         vmw_resource_mob_detach(res);
> +                       if (res->dirty)
> +                               res->func->dirty_free(res);
>                         if (res->coherent)
>                                 vmw_bo_dirty_release(res->guest_memory_bo);
>                         res->guest_memory_bo = NULL;
>                         res->guest_memory_offset = 0;
> -                       vmw_resource_unreserve(res, false, false, false, NULL,
> +                       vmw_resource_unreserve(res, true, false, false, NULL,
>                                                0);
>                         mutex_unlock(&res->dev_priv->cmdbuf_mutex);
>                 }
> @@ -73,9 +75,9 @@ static void vmw_bo_free(struct ttm_buffer_object *bo)
>  {
>         struct vmw_bo *vbo = to_vmw_bo(&bo->base);
>
> -       WARN_ON(vbo->dirty);
>         WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
>         vmw_bo_release(vbo);
> +       WARN_ON(vbo->dirty);
>         kfree(vbo);
>  }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index a73af8a355fb..c4d5fe5f330f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -273,7 +273,7 @@ int vmw_user_resource_lookup_handle(struct vmw_private *dev_priv,
>                 goto out_bad_resource;
>
>         res = converter->base_obj_to_res(base);
> -       kref_get(&res->kref);
> +       vmw_resource_reference(res);
>
>         *p_res = res;
>         ret = 0;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 5721c74da3e0..5e022ed466ae 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -658,7 +658,7 @@ static void vmw_user_surface_free(struct vmw_resource *res)
>         struct vmw_user_surface *user_srf =
>             container_of(srf, struct vmw_user_surface, srf);
>
> -       WARN_ON_ONCE(res->dirty);
> +       WARN_ON(res->dirty);
>         if (user_srf->master)
>                 drm_master_put(&user_srf->master);
>         kfree(srf->offsets);
> @@ -689,8 +689,7 @@ static void vmw_user_surface_base_release(struct ttm_base_object **p_base)
>          * Dumb buffers own the resource and they'll unref the
>          * resource themselves
>          */
> -       if (res && res->guest_memory_bo && res->guest_memory_bo->is_dumb)
> -               return;
> +       WARN_ON(res && res->guest_memory_bo && res->guest_memory_bo->is_dumb);
>
>         vmw_resource_unreference(&res);
>  }
> @@ -2358,12 +2357,19 @@ int vmw_dumb_create(struct drm_file *file_priv,
>         vbo = res->guest_memory_bo;
>         vbo->is_dumb = true;
>         vbo->dumb_surface = vmw_res_to_srf(res);
> -
> +       drm_gem_object_put(&vbo->tbo.base);
> +       /*
> +        * Unset the user surface dtor since this in not actually exposed
> +        * to userspace. The suface is owned via the dumb_buffer's GEM handle
> +        */
> +       struct vmw_user_surface *usurf = container_of(vbo->dumb_surface,
> +                                               struct vmw_user_surface, srf);
> +       usurf->prime.base.refcount_release = NULL;
>  err:
>         if (res)
>                 vmw_resource_unreference(&res);
> -       if (ret)
> -               ttm_ref_object_base_unref(tfile, arg.rep.handle);
> +
> +       ttm_ref_object_base_unref(tfile, arg.rep.handle);
>
>         return ret;
>  }
> --
> 2.43.0
>

This version looks good. Thanks for tackling this!

Reviewed-by: Zack Rusin <zack.rusin at broadcom.com>

z
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5414 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250318/349579f1/attachment.bin>