On Tue, May 22, 2012 at 4:05 PM, Daniel Vetter <daniel at ffwll.ch> wrote: > On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski > <t.stanislaws at 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.
