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 ;-) -Daniel -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
