All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] dma-buf: add get_dma_buf()
@ 2012-03-16 16:04 Rob Clark
  2012-03-16 17:53 ` [Linaro-mm-sig] " Dave Airlie
  2012-05-22 13:47 ` Tomasz Stanislawski
  0 siblings, 2 replies; 14+ messages in thread
From: Rob Clark @ 2012-03-16 16:04 UTC (permalink / raw)
  To: linaro-mm-sig, dri-devel, linux-media
  Cc: patches, sumit.semwal, daniel, airlied, Rob Clark

From: Rob Clark <rob@ti.com>

Works in a similar way to get_file(), and is needed in cases such as
when the exporter needs to also keep a reference to the dmabuf (that
is later released with a dma_buf_put()), and possibly other similar
cases.

Signed-off-by: Rob Clark <rob@ti.com>
---
 include/linux/dma-buf.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index cbdff81..25eb287 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -132,6 +132,20 @@ struct dma_buf_attachment {
 	void *priv;
 };
 
+/**
+ * get_dma_buf - convenience wrapper for get_file.
+ * @dmabuf:	[in]	pointer to dma_buf
+ *
+ * Increments the reference count on the dma-buf, needed in case of drivers
+ * that either need to create additional references to the dmabuf on the
+ * kernel side.  For example, an exporter that needs to keep a dmabuf ptr
+ * so that subsequent exports don't create a new dmabuf.
+ */
+static inline void get_dma_buf(struct dma_buf *dmabuf)
+{
+	get_file(dmabuf->file);
+}
+
 #ifdef CONFIG_DMA_SHARED_BUFFER
 struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 							struct device *dev);
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Linaro-mm-sig] [PATCH] dma-buf: add get_dma_buf()
  2012-03-16 16:04 [PATCH] dma-buf: add get_dma_buf() Rob Clark
@ 2012-03-16 17:53 ` Dave Airlie
  2012-03-18  7:42   ` Sumit Semwal
  2012-05-22 13:47 ` Tomasz Stanislawski
  1 sibling, 1 reply; 14+ messages in thread
From: Dave Airlie @ 2012-03-16 17:53 UTC (permalink / raw)
  To: Rob Clark; +Cc: linaro-mm-sig, dri-devel, linux-media, airlied, daniel, patches

On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
>
> Works in a similar way to get_file(), and is needed in cases such as
> when the exporter needs to also keep a reference to the dmabuf (that
> is later released with a dma_buf_put()), and possibly other similar
> cases.
>
> Signed-off-by: Rob Clark <rob@ti.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] dma-buf: add get_dma_buf()
@ 2012-03-16 20:02 ` Rob Clark
  0 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2012-03-16 20:02 UTC (permalink / raw)
  To: linaro-mm-sig, dri-devel, linux-media
  Cc: airlied, Rob Clark, sumit.semwal, patches

From: Rob Clark <rob@ti.com>

Works in a similar way to get_file(), and is needed in cases such as
when the exporter needs to also keep a reference to the dmabuf (that
is later released with a dma_buf_put()), and possibly other similar
cases.

Signed-off-by: Rob Clark <rob@ti.com>
---
Minor update on original to add a missing #include

 include/linux/dma-buf.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 891457a..bc4203dc 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -30,6 +30,7 @@
 #include <linux/scatterlist.h>
 #include <linux/list.h>
 #include <linux/dma-mapping.h>
+#include <linux/fs.h>
 
 struct dma_buf;
 struct dma_buf_attachment;
@@ -110,6 +111,20 @@ struct dma_buf_attachment {
 	void *priv;
 };
 
+/**
+ * get_dma_buf - convenience wrapper for get_file.
+ * @dmabuf:	[in]	pointer to dma_buf
+ *
+ * Increments the reference count on the dma-buf, needed in case of drivers
+ * that either need to create additional references to the dmabuf on the
+ * kernel side.  For example, an exporter that needs to keep a dmabuf ptr
+ * so that subsequent exports don't create a new dmabuf.
+ */
+static inline void get_dma_buf(struct dma_buf *dmabuf)
+{
+	get_file(dmabuf->file);
+}
+
 #ifdef CONFIG_DMA_SHARED_BUFFER
 struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 							struct device *dev);
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH] dma-buf: add get_dma_buf()
@ 2012-03-16 20:02 ` Rob Clark
  0 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2012-03-16 20:02 UTC (permalink / raw)
  To: linaro-mm-sig, dri-devel, linux-media
  Cc: patches, sumit.semwal, daniel, airlied, Rob Clark

From: Rob Clark <rob@ti.com>

Works in a similar way to get_file(), and is needed in cases such as
when the exporter needs to also keep a reference to the dmabuf (that
is later released with a dma_buf_put()), and possibly other similar
cases.

Signed-off-by: Rob Clark <rob@ti.com>
---
Minor update on original to add a missing #include

 include/linux/dma-buf.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 891457a..bc4203dc 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -30,6 +30,7 @@
 #include <linux/scatterlist.h>
 #include <linux/list.h>
 #include <linux/dma-mapping.h>
+#include <linux/fs.h>
 
 struct dma_buf;
 struct dma_buf_attachment;
@@ -110,6 +111,20 @@ struct dma_buf_attachment {
 	void *priv;
 };
 
+/**
+ * get_dma_buf - convenience wrapper for get_file.
+ * @dmabuf:	[in]	pointer to dma_buf
+ *
+ * Increments the reference count on the dma-buf, needed in case of drivers
+ * that either need to create additional references to the dmabuf on the
+ * kernel side.  For example, an exporter that needs to keep a dmabuf ptr
+ * so that subsequent exports don't create a new dmabuf.
+ */
+static inline void get_dma_buf(struct dma_buf *dmabuf)
+{
+	get_file(dmabuf->file);
+}
+
 #ifdef CONFIG_DMA_SHARED_BUFFER
 struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 							struct device *dev);
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Linaro-mm-sig] [PATCH] dma-buf: add get_dma_buf()
  2012-03-16 17:53 ` [Linaro-mm-sig] " Dave Airlie
@ 2012-03-18  7:42   ` Sumit Semwal
  2012-03-18 19:04     ` Daniel Vetter
  0 siblings, 1 reply; 14+ messages in thread
From: Sumit Semwal @ 2012-03-18  7:42 UTC (permalink / raw)
  To: Dave Airlie
  Cc: Rob Clark, patches, dri-devel, linaro-mm-sig, daniel, airlied,
	linux-media

On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote:
> On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote:
>> From: Rob Clark <rob@ti.com>
>>
>> Works in a similar way to get_file(), and is needed in cases such as
>> when the exporter needs to also keep a reference to the dmabuf (that
>> is later released with a dma_buf_put()), and possibly other similar
>> cases.
>>
>> Signed-off-by: Rob Clark <rob@ti.com>
>
> Reviewed-by: Dave Airlie <airlied@redhat.com>
>
Thanks; pulled into for-next.

BR,
~me.
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Linaro-mm-sig] [PATCH] dma-buf: add get_dma_buf()
  2012-03-18  7:42   ` Sumit Semwal
@ 2012-03-18 19:04     ` Daniel Vetter
  2012-03-18 19:15         ` Daniel Vetter
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2012-03-18 19:04 UTC (permalink / raw)
  To: Sumit Semwal
  Cc: Dave Airlie, Rob Clark, patches, dri-devel, linaro-mm-sig, daniel,
	airlied, linux-media

On Sun, Mar 18, 2012 at 01:12:22PM +0530, Sumit Semwal wrote:
> On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote:
> > On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote:
> >> From: Rob Clark <rob@ti.com>
> >>
> >> Works in a similar way to get_file(), and is needed in cases such as
> >> when the exporter needs to also keep a reference to the dmabuf (that
> >> is later released with a dma_buf_put()), and possibly other similar
> >> cases.
> >>
> >> Signed-off-by: Rob Clark <rob@ti.com>
> >
> > Reviewed-by: Dave Airlie <airlied@redhat.com>
> >
> Thanks; pulled into for-next.

I'm back from vacation and already grumpily complaining about dma-buf
patches ;-) For consistency with dma_buf_put we should call this
dma_buf_get instead of get_dma_buf ... I'll write a bikeshed patch on top
of your tree.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Linaro-mm-sig] [PATCH] dma-buf: add get_dma_buf()
  2012-03-18 19:04     ` Daniel Vetter
@ 2012-03-18 19:15         ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2012-03-18 19:15 UTC (permalink / raw)
  To: Sumit Semwal
  Cc: patches, dri-devel, Rob Clark, linaro-mm-sig, airlied,
	linux-media

On Sun, Mar 18, 2012 at 08:04:53PM +0100, Daniel Vetter wrote:
> On Sun, Mar 18, 2012 at 01:12:22PM +0530, Sumit Semwal wrote:
> > On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote:
> > > On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote:
> > >> From: Rob Clark <rob@ti.com>
> > >>
> > >> Works in a similar way to get_file(), and is needed in cases such as
> > >> when the exporter needs to also keep a reference to the dmabuf (that
> > >> is later released with a dma_buf_put()), and possibly other similar
> > >> cases.
> > >>
> > >> Signed-off-by: Rob Clark <rob@ti.com>
> > >
> > > Reviewed-by: Dave Airlie <airlied@redhat.com>
> > >
> > Thanks; pulled into for-next.
> 
> I'm back from vacation and already grumpily complaining about dma-buf
> patches ;-) For consistency with dma_buf_put we should call this
> dma_buf_get instead of get_dma_buf ... I'll write a bikeshed patch on top
> of your tree.

Oops, there's already a dma_buf_get around - Rob and Dave pointed that out
on irc to dense me.  And I can't come up with a saner naming scheme. I'll
retract my bikeshed.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Linaro-mm-sig] [PATCH] dma-buf: add get_dma_buf()
@ 2012-03-18 19:15         ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2012-03-18 19:15 UTC (permalink / raw)
  To: Sumit Semwal
  Cc: Dave Airlie, Rob Clark, patches, dri-devel, linaro-mm-sig, daniel,
	airlied, linux-media

On Sun, Mar 18, 2012 at 08:04:53PM +0100, Daniel Vetter wrote:
> On Sun, Mar 18, 2012 at 01:12:22PM +0530, Sumit Semwal wrote:
> > On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote:
> > > On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote:
> > >> From: Rob Clark <rob@ti.com>
> > >>
> > >> Works in a similar way to get_file(), and is needed in cases such as
> > >> when the exporter needs to also keep a reference to the dmabuf (that
> > >> is later released with a dma_buf_put()), and possibly other similar
> > >> cases.
> > >>
> > >> Signed-off-by: Rob Clark <rob@ti.com>
> > >
> > > Reviewed-by: Dave Airlie <airlied@redhat.com>
> > >
> > Thanks; pulled into for-next.
> 
> I'm back from vacation and already grumpily complaining about dma-buf
> patches ;-) For consistency with dma_buf_put we should call this
> dma_buf_get instead of get_dma_buf ... I'll write a bikeshed patch on top
> of your tree.

Oops, there's already a dma_buf_get around - Rob and Dave pointed that out
on irc to dense me.  And I can't come up with a saner naming scheme. I'll
retract my bikeshed.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] dma-buf: add get_dma_buf()
  2012-03-16 16:04 [PATCH] dma-buf: add get_dma_buf() Rob Clark
  2012-03-16 17:53 ` [Linaro-mm-sig] " Dave Airlie
@ 2012-05-22 13:47 ` Tomasz Stanislawski
  2012-05-22 14:32   ` Daniel Vetter
  1 sibling, 1 reply; 14+ messages in thread
From: Tomasz Stanislawski @ 2012-05-22 13:47 UTC (permalink / raw)
  To: Rob Clark
  Cc: linaro-mm-sig, dri-devel, linux-media, patches, sumit.semwal,
	daniel, airlied, Rob Clark, Marek Szyprowski,
	'박경민', InKi Dae

Hi,
I think I discovered an interesting issue with dma_buf.
I found out that dma_buf_fd does not increase reference
count for dma_buf::file. This leads to potential kernel
crash triggered by user space. Please, take a look on
the scenario below:

The applications spawns two thread. One of them is exporting DMABUF.

      Thread I         |   Thread II       | Comments
-----------------------+-------------------+-----------------------------------
dbuf = dma_buf_export  |                   | dma_buf is creates, refcount is 1
fd = dma_buf_fd(dbuf)  |                   | assume fd is set to 42, refcount is still 1
                       |      close(42)    | The file descriptor is closed asynchronously, dbuf's refcount drops to 0
                       |  dma_buf_release  | dbuf structure is freed, dbuf becomes a dangling pointer
int size = dbuf->size; |                   | the dbuf is dereferenced, causing a kernel crash
-----------------------+-------------------+-----------------------------------

I think that the problem could be fixed in two ways.
a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd.
b) increasing dma_buf->file's reference count at dma_buf_fd

I prefer solution (b) because it prevents symmetry between dma_buf_fd and close.
I mean that dma_buf_fd increases reference count, close decreases it.

What is your opinion about the issue?

Regards,
Tomasz Stanislawski



On 03/16/2012 05:04 PM, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> Works in a similar way to get_file(), and is needed in cases such as
> when the exporter needs to also keep a reference to the dmabuf (that
> is later released with a dma_buf_put()), and possibly other similar
> cases.
> 
> Signed-off-by: Rob Clark <rob@ti.com>
> ---

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] dma-buf: add get_dma_buf()
  2012-05-22 13:47 ` Tomasz Stanislawski
@ 2012-05-22 14:32   ` Daniel Vetter
  2012-05-22 15:00     ` Tomasz Stanislawski
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2012-05-22 14:32 UTC (permalink / raw)
  To: Tomasz Stanislawski
  Cc: Rob Clark, linaro-mm-sig, dri-devel, linux-media, patches,
	sumit.semwal, daniel, airlied, Rob Clark, Marek Szyprowski,
	'박경민', InKi Dae

On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote:
> Hi,
> I think I discovered an interesting issue with dma_buf.
> I found out that dma_buf_fd does not increase reference
> count for dma_buf::file. This leads to potential kernel
> crash triggered by user space. Please, take a look on
> the scenario below:
> 
> The applications spawns two thread. One of them is exporting DMABUF.
> 
>       Thread I         |   Thread II       | Comments
> -----------------------+-------------------+-----------------------------------
> dbuf = dma_buf_export  |                   | dma_buf is creates, refcount is 1
> fd = dma_buf_fd(dbuf)  |                   | assume fd is set to 42, refcount is still 1
>                        |      close(42)    | The file descriptor is closed asynchronously, dbuf's refcount drops to 0
>                        |  dma_buf_release  | dbuf structure is freed, dbuf becomes a dangling pointer
> int size = dbuf->size; |                   | the dbuf is dereferenced, causing a kernel crash
> -----------------------+-------------------+-----------------------------------
> 
> I think that the problem could be fixed in two ways.
> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd.
> b) increasing dma_buf->file's reference count at dma_buf_fd
> 
> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close.
> I mean that dma_buf_fd increases reference count, close decreases it.
> 
> What is your opinion about the issue?

I guess most exporters would like to hang onto the exported dma_buf a bit
and hence need a reference (e.g. to cache the dma_buf as long as the
underlying buffer object exists). So I guess we can change the semantics
of dma_buf_fd from transferring the reference you currently have (and
hence forbidding any further access by the caller) to grabbing a reference
of it's on for the fd that is created.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] dma-buf: add get_dma_buf()
  2012-05-22 14:32   ` Daniel Vetter
@ 2012-05-22 15:00     ` Tomasz Stanislawski
  2012-05-22 15:05       ` Daniel Vetter
  0 siblings, 1 reply; 14+ messages in thread
From: Tomasz Stanislawski @ 2012-05-22 15:00 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Rob Clark, linaro-mm-sig, dri-devel, linux-media, patches,
	sumit.semwal, airlied, Rob Clark, Marek Szyprowski,
	'박경민', InKi Dae

On 05/22/2012 04:32 PM, Daniel Vetter wrote:
> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote:
>> Hi,
>> I think I discovered an interesting issue with dma_buf.
>> I found out that dma_buf_fd does not increase reference
>> count for dma_buf::file. This leads to potential kernel
>> crash triggered by user space. Please, take a look on
>> the scenario below:
>>
>> The applications spawns two thread. One of them is exporting DMABUF.
>>
>>       Thread I         |   Thread II       | Comments
>> -----------------------+-------------------+-----------------------------------
>> dbuf = dma_buf_export  |                   | dma_buf is creates, refcount is 1
>> fd = dma_buf_fd(dbuf)  |                   | assume fd is set to 42, refcount is still 1
>>                        |      close(42)    | The file descriptor is closed asynchronously, dbuf's refcount drops to 0
>>                        |  dma_buf_release  | dbuf structure is freed, dbuf becomes a dangling pointer
>> int size = dbuf->size; |                   | the dbuf is dereferenced, causing a kernel crash
>> -----------------------+-------------------+-----------------------------------
>>
>> I think that the problem could be fixed in two ways.
>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd.
>> b) increasing dma_buf->file's reference count at dma_buf_fd
>>
>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close.
>> I mean that dma_buf_fd increases reference count, close decreases it.
>>
>> What is your opinion about the issue?
> 
> I guess most exporters would like to hang onto the exported dma_buf a bit
> and hence need a reference (e.g. to cache the dma_buf as long as the
> underlying buffer object exists). So I guess we can change the semantics
> of dma_buf_fd from transferring the reference you currently have (and
> hence forbidding any further access by the caller) to grabbing a reference
> of it's on for the fd that is created.
> -Daniel

Hi Daniel,
Would it be simpler, safer and more intuitive if dma_buf_fd increased
dmabuf->file's reference counter?

Regards,
Tomasz Stanislawski

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] dma-buf: add get_dma_buf()
  2012-05-22 15:00     ` Tomasz Stanislawski
@ 2012-05-22 15:05       ` Daniel Vetter
  2012-05-22 15:13         ` Dave Airlie
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2012-05-22 15:05 UTC (permalink / raw)
  To: Tomasz Stanislawski
  Cc: Rob Clark, linaro-mm-sig, dri-devel, linux-media, patches,
	sumit.semwal, airlied, Rob Clark, Marek Szyprowski,
	박경민, InKi Dae

On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
> On 05/22/2012 04:32 PM, Daniel Vetter wrote:
>> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote:
>>> Hi,
>>> I think I discovered an interesting issue with dma_buf.
>>> I found out that dma_buf_fd does not increase reference
>>> count for dma_buf::file. This leads to potential kernel
>>> crash triggered by user space. Please, take a look on
>>> the scenario below:
>>>
>>> The applications spawns two thread. One of them is exporting DMABUF.
>>>
>>>       Thread I         |   Thread II       | Comments
>>> -----------------------+-------------------+-----------------------------------
>>> dbuf = dma_buf_export  |                   | dma_buf is creates, refcount is 1
>>> fd = dma_buf_fd(dbuf)  |                   | assume fd is set to 42, refcount is still 1
>>>                        |      close(42)    | The file descriptor is closed asynchronously, dbuf's refcount drops to 0
>>>                        |  dma_buf_release  | dbuf structure is freed, dbuf becomes a dangling pointer
>>> int size = dbuf->size; |                   | the dbuf is dereferenced, causing a kernel crash
>>> -----------------------+-------------------+-----------------------------------
>>>
>>> I think that the problem could be fixed in two ways.
>>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd.
>>> b) increasing dma_buf->file's reference count at dma_buf_fd
>>>
>>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close.
>>> I mean that dma_buf_fd increases reference count, close decreases it.
>>>
>>> What is your opinion about the issue?
>>
>> I guess most exporters would like to hang onto the exported dma_buf a bit
>> and hence need a reference (e.g. to cache the dma_buf as long as the
>> underlying buffer object exists). So I guess we can change the semantics
>> of dma_buf_fd from transferring the reference you currently have (and
>> hence forbidding any further access by the caller) to grabbing a reference
>> of it's on for the fd that is created.
>> -Daniel
>
> Hi Daniel,
> Would it be simpler, safer and more intuitive if dma_buf_fd increased
> dmabuf->file's reference counter?

That's actually what I wanted to say. Message seems to have been lost
in transit ;-)
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] dma-buf: add get_dma_buf()
  2012-05-22 15:05       ` Daniel Vetter
@ 2012-05-22 15:13         ` Dave Airlie
  2012-05-22 17:37           ` Rob Clark
  0 siblings, 1 reply; 14+ messages in thread
From: Dave Airlie @ 2012-05-22 15:13 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Tomasz Stanislawski, patches, dri-devel, Rob Clark, linaro-mm-sig,
	박경민, Rob Clark, airlied, Marek Szyprowski,
	sumit.semwal, linux-media

On Tue, May 22, 2012 at 4:05 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski
> <t.stanislaws@samsung.com> wrote:
>> On 05/22/2012 04:32 PM, Daniel Vetter wrote:
>>> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote:
>>>> Hi,
>>>> I think I discovered an interesting issue with dma_buf.
>>>> I found out that dma_buf_fd does not increase reference
>>>> count for dma_buf::file. This leads to potential kernel
>>>> crash triggered by user space. Please, take a look on
>>>> the scenario below:
>>>>
>>>> The applications spawns two thread. One of them is exporting DMABUF.
>>>>
>>>>       Thread I         |   Thread II       | Comments
>>>> -----------------------+-------------------+-----------------------------------
>>>> dbuf = dma_buf_export  |                   | dma_buf is creates, refcount is 1
>>>> fd = dma_buf_fd(dbuf)  |                   | assume fd is set to 42, refcount is still 1
>>>>                        |      close(42)    | The file descriptor is closed asynchronously, dbuf's refcount drops to 0
>>>>                        |  dma_buf_release  | dbuf structure is freed, dbuf becomes a dangling pointer
>>>> int size = dbuf->size; |                   | the dbuf is dereferenced, causing a kernel crash
>>>> -----------------------+-------------------+-----------------------------------
>>>>
>>>> I think that the problem could be fixed in two ways.
>>>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd.
>>>> b) increasing dma_buf->file's reference count at dma_buf_fd
>>>>
>>>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close.
>>>> I mean that dma_buf_fd increases reference count, close decreases it.
>>>>
>>>> What is your opinion about the issue?
>>>
>>> I guess most exporters would like to hang onto the exported dma_buf a bit
>>> and hence need a reference (e.g. to cache the dma_buf as long as the
>>> underlying buffer object exists). So I guess we can change the semantics
>>> of dma_buf_fd from transferring the reference you currently have (and
>>> hence forbidding any further access by the caller) to grabbing a reference
>>> of it's on for the fd that is created.
>>> -Daniel
>>
>> Hi Daniel,
>> Would it be simpler, safer and more intuitive if dma_buf_fd increased
>> dmabuf->file's reference counter?
>
> That's actually what I wanted to say. Message seems to have been lost
> in transit ;-)

Now I've thought about it and Tomasz has pointed it out I agree,

Now we just have to work out when to drop that reference, which I
don't see anyone addressing :-)

I love lifetime rules.

Dave.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] dma-buf: add get_dma_buf()
  2012-05-22 15:13         ` Dave Airlie
@ 2012-05-22 17:37           ` Rob Clark
  0 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2012-05-22 17:37 UTC (permalink / raw)
  To: Dave Airlie
  Cc: Daniel Vetter, Tomasz Stanislawski, patches, dri-devel,
	linaro-mm-sig, 박경민, Rob Clark, airlied,
	Marek Szyprowski, sumit.semwal, linux-media

On Tue, May 22, 2012 at 9:13 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Tue, May 22, 2012 at 4:05 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski
>> <t.stanislaws@samsung.com> wrote:
>>> On 05/22/2012 04:32 PM, Daniel Vetter wrote:
>>>> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote:
>>>>> Hi,
>>>>> I think I discovered an interesting issue with dma_buf.
>>>>> I found out that dma_buf_fd does not increase reference
>>>>> count for dma_buf::file. This leads to potential kernel
>>>>> crash triggered by user space. Please, take a look on
>>>>> the scenario below:
>>>>>
>>>>> The applications spawns two thread. One of them is exporting DMABUF.
>>>>>
>>>>>       Thread I         |   Thread II       | Comments
>>>>> -----------------------+-------------------+-----------------------------------
>>>>> dbuf = dma_buf_export  |                   | dma_buf is creates, refcount is 1
>>>>> fd = dma_buf_fd(dbuf)  |                   | assume fd is set to 42, refcount is still 1
>>>>>                        |      close(42)    | The file descriptor is closed asynchronously, dbuf's refcount drops to 0
>>>>>                        |  dma_buf_release  | dbuf structure is freed, dbuf becomes a dangling pointer
>>>>> int size = dbuf->size; |                   | the dbuf is dereferenced, causing a kernel crash
>>>>> -----------------------+-------------------+-----------------------------------
>>>>>
>>>>> I think that the problem could be fixed in two ways.
>>>>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd.
>>>>> b) increasing dma_buf->file's reference count at dma_buf_fd
>>>>>
>>>>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close.
>>>>> I mean that dma_buf_fd increases reference count, close decreases it.
>>>>>
>>>>> What is your opinion about the issue?
>>>>
>>>> I guess most exporters would like to hang onto the exported dma_buf a bit
>>>> and hence need a reference (e.g. to cache the dma_buf as long as the
>>>> underlying buffer object exists). So I guess we can change the semantics
>>>> of dma_buf_fd from transferring the reference you currently have (and
>>>> hence forbidding any further access by the caller) to grabbing a reference
>>>> of it's on for the fd that is created.
>>>> -Daniel
>>>
>>> Hi Daniel,
>>> Would it be simpler, safer and more intuitive if dma_buf_fd increased
>>> dmabuf->file's reference counter?
>>
>> That's actually what I wanted to say. Message seems to have been lost
>> in transit ;-)
>
> Now I've thought about it and Tomasz has pointed it out I agree,
>
> Now we just have to work out when to drop that reference, which I
> don't see anyone addressing :-)
>
> I love lifetime rules.

I think in the GEM case, where we keep a ref in obj->export_dma_buf,
we can drop the extra ref to the dmabuf (if we decide dma_buf_fd()
increases the refcnt), as long as we be sure to NULL out
obj->export_dma_buf from dmabuf_ops->release (which is unfortunately
in each driver at the moment)..  This way obj->export_dma_buf behaves
as a sort of weak-reference, not causing a memory leak.

BR,
-R

> Dave.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-05-22 17:37 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-16 16:04 [PATCH] dma-buf: add get_dma_buf() Rob Clark
2012-03-16 17:53 ` [Linaro-mm-sig] " Dave Airlie
2012-03-18  7:42   ` Sumit Semwal
2012-03-18 19:04     ` Daniel Vetter
2012-03-18 19:15       ` Daniel Vetter
2012-03-18 19:15         ` Daniel Vetter
2012-05-22 13:47 ` Tomasz Stanislawski
2012-05-22 14:32   ` Daniel Vetter
2012-05-22 15:00     ` Tomasz Stanislawski
2012-05-22 15:05       ` Daniel Vetter
2012-05-22 15:13         ` Dave Airlie
2012-05-22 17:37           ` Rob Clark
  -- strict thread matches above, loose matches on Subject: below --
2012-03-16 20:02 Rob Clark
2012-03-16 20:02 ` Rob Clark

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.