* [PATCH] drm/prime: drop reference on imported dma-buf
@ 2012-07-23 23:07 Rob Clark
2012-07-24 6:17 ` Paul Menzel
2012-07-24 8:05 ` Daniel Vetter
0 siblings, 2 replies; 6+ messages in thread
From: Rob Clark @ 2012-07-23 23:07 UTC (permalink / raw)
To: dri-devel; +Cc: Rob Clark, patches
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
+ * 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf
2012-07-23 23:07 [PATCH] drm/prime: drop reference on imported dma-buf Rob Clark
@ 2012-07-24 6:17 ` Paul Menzel
2012-07-24 8:05 ` Daniel Vetter
1 sibling, 0 replies; 6+ messages in thread
From: Paul Menzel @ 2012-07-24 6:17 UTC (permalink / raw)
To: Rob Clark; +Cc: Rob Clark, dri-devel, patches
[-- Attachment #1.1: Type: text/plain, Size: 1498 bytes --]
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
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf
2012-07-23 23:07 [PATCH] drm/prime: drop reference on imported dma-buf Rob Clark
2012-07-24 6:17 ` Paul Menzel
@ 2012-07-24 8:05 ` Daniel Vetter
2012-09-04 22:32 ` Rob Clark
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2012-07-24 8:05 UTC (permalink / raw)
To: Rob Clark; +Cc: Rob Clark, dri-devel, patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf
2012-07-24 8:05 ` Daniel Vetter
@ 2012-09-04 22:32 ` Rob Clark
2012-09-05 9:02 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Rob Clark @ 2012-09-04 22:32 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel, patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf
2012-09-04 22:32 ` Rob Clark
@ 2012-09-05 9:02 ` Daniel Vetter
2012-09-05 12:01 ` Rob Clark
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2012-09-05 9:02 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel, patches
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
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf
2012-09-05 9:02 ` Daniel Vetter
@ 2012-09-05 12:01 ` Rob Clark
0 siblings, 0 replies; 6+ messages in thread
From: Rob Clark @ 2012-09-05 12:01 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel, patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-05 12:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-23 23:07 [PATCH] drm/prime: drop reference on imported dma-buf Rob Clark
2012-07-24 6:17 ` Paul Menzel
2012-07-24 8:05 ` Daniel Vetter
2012-09-04 22:32 ` Rob Clark
2012-09-05 9:02 ` Daniel Vetter
2012-09-05 12:01 ` Rob Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox