Hi Tomasz,
On Wednesday 20 June 2012 13:51:06 Tomasz Stanislawski wrote:
On 06/19/2012 11:00 PM, Laurent Pinchart wrote:
On Thursday 14 June 2012 15:37:40 Tomasz Stanislawski wrote:
This patch removes a reference to alloc_ctx from an instance of a DMA contiguous buffer. It helps to avoid a risk of a dangling pointer if the context is released while the buffer is still valid.
Can this really happen ? All drivers except marvell-ccic seem to call vb2_dma_contig_cleanup_ctx() in their remove handler and probe cleanup path only. Freeing the context while buffers are still around would be a driver bug, and I expect drivers to destroy the queue in that case anyway.
This being said, removing the dereference step is a good idea, so I think the patch should be applied, possibly with a different commit message.
The problem may happen if a DMABUF sharing is used.
- process A uses V4L2 queue to create a buffer
- process A exports a buffer and shares it with the process B (by sockets or
/proc/pid/fd) - the process A gets killed, queue is destroyed
- someone call rmmod on v4l driver, alloc_ctx is freed
That's where the problem is in my opinion. As long as a buffer is in use, the queue should not get destroyed and the context should not be freed. If the driver is removed it will kfree the structure that embeds the queue, and even possible free the whole memory region that backs the buffers. We would then have much bigger trouble than just a dangling context pointer.
From a V4L2 point of view this needs to be solved for the dmabuf exporter role
only, so it's not a huge concern in the context of this patch set, but we of course need to address the issue.
- process B keeps reference to a buffer that has a dangling reference to
alloc_ctx
The presented scenario might be a bit too pathological and artificial. Moreover it involves root privileges. But it is possible to trigger this bug. One solution might be keeping reference count in alloc_ctx but it would be easier to get rid of the reference to alloc_ctx from vb2-dma-contig buffer.
BTW. I decided to drop 'Remove unneeded allocation context structure' because Marek Szyprowski is working on extension to vb2-dma-contig that allow to create buffers with no kernel mappings. That feature involved additional parameter to alloc_ctx other than pointer to the device.
OK.