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