Re: [RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

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;
>>>> +}
>