Re: [PATCH next] drm/syncobj: Fix leak in drm_syncobj_import_sync_file_fence()

[Rob Clark]

On Thu, Apr 10, 2025 at 9:33 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> We need to cleanup if the chain = dma_fence_chain_alloc() allocation
> fails.  Now that we have multiple error returns in this function, switch
> to using an unwind ladder for cleanup.
>
> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 636cd83ca29e..c136d0c772dc 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -745,21 +745,24 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  {
>         struct dma_fence *fence = sync_file_get_fence(fd);
>         struct drm_syncobj *syncobj;
> +       int ret;
>
>         if (!fence)
>                 return -EINVAL;
>
>         syncobj = drm_syncobj_find(file_private, handle);
>         if (!syncobj) {
> -               dma_fence_put(fence);
> -               return -ENOENT;
> +               ret = -ENOENT;
> +               goto err_put_fence;
>         }
>
>         if (point) {
>                 struct dma_fence_chain *chain = dma_fence_chain_alloc();
>
> -               if (!chain)
> -                       return -ENOMEM;
> +               if (!chain) {
> +                       ret = -ENOMEM;
> +                       goto err_put_syncobj;
> +               }
>
>                 drm_syncobj_add_point(syncobj, chain, fence, point);
>         } else {
> @@ -769,6 +772,13 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>         dma_fence_put(fence);
>         drm_syncobj_put(syncobj);
>         return 0;
> +
> +err_put_syncobj:
> +       drm_syncobj_put(syncobj);
> +err_put_fence:
> +       dma_fence_put(fence);
> +
> +       return ret;

I suppose you could just initialize ret to zero and collapse the two
return paths?  Either way,

Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>

>  }
>
>  static int drm_syncobj_export_sync_file(struct drm_file *file_private,
> --
> 2.47.2
>