Re: [PATCH] dma-buf: add get_dma_buf()

[Rob Clark <rob.clark@xxxxxxxxxx>]

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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel