[PATCHv7,06/15] v4l: vb2-dma-contig: remove reference of alloc_ctx from a buffer

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. Moreover it removes one
dereference step while accessing a device structure.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Hi Tomasz,

Thanks for the patch.

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.

> Moreover it removes one
> dereference step while accessing a device structure.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/video/videobuf2-dma-contig.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index a05784f..20c95da 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c
> @@ -23,7 +23,7 @@ struct vb2_dc_conf {
>  };
> 
>  struct vb2_dc_buf {
> -	struct vb2_dc_conf		*conf;
> +	struct device			*dev;
>  	void				*vaddr;
>  	dma_addr_t			dma_addr;
>  	unsigned long			size;
> @@ -37,22 +37,21 @@ static void vb2_dc_put(void *buf_priv);
>  static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
>  {
>  	struct vb2_dc_conf *conf = alloc_ctx;
> +	struct device *dev = conf->dev;
>  	struct vb2_dc_buf *buf;
> 
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> 
> -	buf->vaddr = dma_alloc_coherent(conf->dev, size, &buf->dma_addr,
> -					GFP_KERNEL);
> +	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);
>  	if (!buf->vaddr) {
> -		dev_err(conf->dev, "dma_alloc_coherent of size %ld failed\n",
> -			size);
> +		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
>  		kfree(buf);
>  		return ERR_PTR(-ENOMEM);
>  	}
> 
> -	buf->conf = conf;
> +	buf->dev = dev;
>  	buf->size = size;
> 
>  	buf->handler.refcount = &buf->refcount;
> @@ -69,7 +68,7 @@ static void vb2_dc_put(void *buf_priv)
>  	struct vb2_dc_buf *buf = buf_priv;
> 
>  	if (atomic_dec_and_test(&buf->refcount)) {
> -		dma_free_coherent(buf->conf->dev, buf->size, buf->vaddr,
> +		dma_free_coherent(buf->dev, buf->size, buf->vaddr,
>  				  buf->dma_addr);
>  		kfree(buf);
>  	}

Hi Laurent,

On 06/19/2012 11:00 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thanks for the patch.
> 
> 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
- 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.

Regards,
Tomasz Stanislawski

>> Moreover it removes one
>> dereference step while accessing a device structure.
>>
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +		dma_free_coherent(buf->dev, buf->size, buf->vaddr,
>>  				  buf->dma_addr);
>>  		kfree(buf);
>>  	}

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.