Re: [PATCH] drm/prime: drop reference on imported dma-buf

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 24, 2012 at 1:07 AM, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
>
> The GEM handle takes the reference.  If a driver is actually importing a
> foreign dmabuf, rather than just re-importing it's own dmabuf, it needs
> to do a get_dma_buf().
>
> Signed-off-by: Rob Clark <rob@ti.com>
[Maybe mention that this is on top of my locking fixes.]

We've discussed this a bit on irc and concluded that every function taking
care of their own references to the dma buf works best in avoiding
potential use-after-free issues when evil userspaces sneaks in a
close(dma_buf_fd).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_prime.c                |    7 +++++++
>  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |    4 ++++
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c     |    4 ++++
>  drivers/gpu/drm/nouveau/nouveau_prime.c    |    5 +++++
>  drivers/gpu/drm/radeon/radeon_prime.c      |    4 ++++
>  drivers/gpu/drm/udl/udl_gem.c              |    4 ++++
>  6 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 4f80374..088e018 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -185,6 +185,13 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>         mutex_unlock(&file_priv->prime.lock);
>         drm_gem_object_unreference_unlocked(obj);
>
> +       /*
> +        * Drop the ref we obtained w/ dma_buf_get() for the benefit of
> +        * drivers simply re-importing their own dma-buf.. if drivers
> +        * import a foreign dma-buf, they should get_dma_buf().
> +        */
> +       dma_buf_put(dma_buf);
> +
>         return 0;
>
>  fail:
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
> index f270790..f8ac75b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
> @@ -190,6 +190,9 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
>                 }
>         }
>
> +       /* since we are attaching, we need to hold a ref */
> +       get_dma_buf(dma_buf);
> +
>         attach = dma_buf_attach(dma_buf, drm_dev->dev);
>         if (IS_ERR(attach))
>                 return ERR_PTR(-EINVAL);
> @@ -250,6 +253,7 @@ err_unmap_attach:
>         dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
>  err_buf_detach:
>         dma_buf_detach(dma_buf, attach);
> +       dma_buf_put(dma_buf);
>         return ERR_PTR(ret);
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index cf6bdec..9d2b2eb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -189,6 +189,9 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>                 }
>         }
>
> +       /* since we are attaching, we need to hold a ref */
> +       get_dma_buf(dma_buf);
> +
>         /* need to attach */
>         attach = dma_buf_attach(dma_buf, dev->dev);
>         if (IS_ERR(attach))
> @@ -224,5 +227,6 @@ fail_unmap:
>         dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
>  fail_detach:
>         dma_buf_detach(dma_buf, attach);
> +       dma_buf_put(dma_buf);
>         return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
> index 4c82801..fc30886 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
> @@ -200,6 +200,10 @@ struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev,
>                         }
>                 }
>         }
> +
> +       /* since we are attaching, we need to hold a ref */
> +       get_dma_buf(dma_buf);
> +
>         /* need to attach */
>         attach = dma_buf_attach(dma_buf, dev->dev);
>         if (IS_ERR(attach))
> @@ -223,6 +227,7 @@ fail_unmap:
>         dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
>  fail_detach:
>         dma_buf_detach(dma_buf, attach);
> +       dma_buf_put(dma_buf);
>         return ERR_PTR(ret);
>  }
>
> diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c
> index 28f79ac..073a8d3 100644
> --- a/drivers/gpu/drm/radeon/radeon_prime.c
> +++ b/drivers/gpu/drm/radeon/radeon_prime.c
> @@ -195,6 +195,9 @@ struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
>                 }
>         }
>
> +       /* since we are attaching, we need to hold a ref */
> +       get_dma_buf(dma_buf);
> +
>         /* need to attach */
>         attach = dma_buf_attach(dma_buf, dev->dev);
>         if (IS_ERR(attach))
> @@ -218,5 +221,6 @@ fail_unmap:
>         dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
>  fail_detach:
>         dma_buf_detach(dma_buf, attach);
> +       dma_buf_put(dma_buf);
>         return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> index 7bd65bd..47f25d5 100644
> --- a/drivers/gpu/drm/udl/udl_gem.c
> +++ b/drivers/gpu/drm/udl/udl_gem.c
> @@ -305,6 +305,9 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>         struct udl_gem_object *uobj;
>         int ret;
>
> +       /* since we are attaching, we need to hold a ref */
> +       get_dma_buf(dma_buf);
> +
>         /* need to attach */
>         attach = dma_buf_attach(dma_buf, dev->dev);
>         if (IS_ERR(attach))
> @@ -329,5 +332,6 @@ fail_unmap:
>         dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
>  fail_detach:
>         dma_buf_detach(dma_buf, attach);
> +       dma_buf_put(dma_buf);
>         return ERR_PTR(ret);
>  }
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch