| Message ID | 1343084868-13427-1-git-send-email-rob.clark@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Rob, Am Montag, den 23.07.2012, 18:07 -0500 schrieb Rob Clark: > 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> > --- > 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 Is that supposed to be an ellipse or a full stop? > + * import a foreign dma-buf, they should get_dma_buf(). Would s,should,should use, be better? > + */ > + dma_buf_put(dma_buf); > + > return 0; > > fail: […] Thanks, Paul
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
Dave, I just noticed that I still have this patch locally, but don't see it in drm-next.. so just checking that it didn't get forgotten BR, -R On Tue, Jul 24, 2012 at 3:05 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 04, 2012 at 05:32:21PM -0500, Rob Clark wrote: > Dave, I just noticed that I still have this patch locally, but don't > see it in drm-next.. so just checking that it didn't get forgotten My locking fixes blew up :( -Daniel > > BR, > -R > > On Tue, Jul 24, 2012 at 3:05 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > 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 > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 5, 2012 at 4:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Sep 04, 2012 at 05:32:21PM -0500, Rob Clark wrote: >> Dave, I just noticed that I still have this patch locally, but don't >> see it in drm-next.. so just checking that it didn't get forgotten > > My locking fixes blew up :( oh, ok.. I guess there was a thread that I missed.. BR, -R > -Daniel > >> >> BR, >> -R >> >> On Tue, Jul 24, 2012 at 3:05 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > 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 >> > _______________________________________________ >> > dri-devel mailing list >> > dri-devel@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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); }