On Tue, May 22, 2012 at 9:13 AM, Dave Airlie <airlied@xxxxxxxxx> wrote: > On Tue, May 22, 2012 at 4:05 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: >> On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski >> <t.stanislaws@xxxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
