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);
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
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
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
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
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
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
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
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
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.
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.
linaro-mm-sig@lists.linaro.org
-
Daniel Vetter -
Dave Airlie -
Rob Clark -
Sumit Semwal -
Tomasz Stanislawski