Hi Laurent,
Sorry for the late reply.
Thank you very much for noticing the issue.
>>>> +static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv)
>>>> +{
>>>> + struct vb2_dc_buf *buf = buf_priv;
>>>> + struct dma_buf *dbuf;
>>>> +
>>>> + if (buf->dma_buf)
>>>> + return buf->dma_buf;
>>>
>>> Can't there be a race condition here if the user closes the DMABUF file
>>> handle before vb2 core calls dma_buf_fd() ?
>>
>> The user cannot access the file until it is associated with a file
>> descriptor. How can the user close it? Could you give me a more detailed
>> description of this potential race condition?
>
> Let's assume the V4L2 buffer has already been exported once. buf->dma_buf is
> set to a non-NULL value, and the application has an open file handle for the
> buffer. The application then tries to export the buffer a second time.
> vb2_dc_get_dmabuf() gets called, checks buf->dma_buf and returns it as it's
> non-NULL. Right after that, before the vb2 core calls dma_buf_fd() on the
> struct dma_buf, the application closes the file handle to the exported
> buffer.
> The struct dma_buf object gets freed, as the reference count drops to 0.
I am not sure if reference counter drops to 0 in this case. Notice that after
first dma_buf_export the dma_buf's refcnt is 1, after first dma_buf_fd it goes
to 2. After closing a file handle the refcnt drops back to 1 so the file
(and dma_buf) is not released. Therefore IMO there no dangling pointer issue.
However it looks that there is a circular reference between vb2_dc_buf and
dma_buf.
vb2_dc_buf::dma_buf is pointing to dma_buf with reference taken at
dma_buf_export.
On the other hand the dma_buf->priv is pointing to vb2_dc_buf with reference
taken at atomic_inc(&buf->refcount) in vb2_dc_get_dmabuf.
The circular reference leads into resource leakage.
The problem could be fixed by dropping caching of dma_buf pointer.
The new dma_buf would be created and exported at every export event.
The dma_buf_put would be called in vb2_expbuf just after
successful dma_buf_fd.
Do you have any ideas how I could deal with resource leakage/dangling problems
without creating a new dma_buf instance at every export event?
Regards,
Tomasz Stanislawski
> vb2 core will then try to call dma_buf_fd() on a dma_buf object that has been
> freed.
>
>>>> + /* dmabuf keeps reference to vb2 buffer */
>>>> + atomic_inc(&buf->refcount);
>>>> + dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
>>>> + if (IS_ERR(dbuf)) {
>>>> + atomic_dec(&buf->refcount);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + buf->dma_buf = dbuf;
>>>> +
>>>> + return dbuf;
>>>> +}
>
