Re: [PATCH v2 6/8] media: platform: amd: isp4 video node and buffers handling added

["Du, Bin" <bin.du@amd.com>]

Many thanks Sultan for your so careful review.

Regards,
Bin

On 7/24/2025 1:55 AM, Sultan Alsawaf wrote:
> On Wed, Jun 18, 2025 at 05:19:57PM +0800, Bin Du wrote:
>> +static void isp4vid_vb2_detach_dmabuf(void *mem_priv)
>> +{
>> +	struct isp4vid_vb2_buf *buf = mem_priv;
>> +
>> +	if (!buf) {
>> +		pr_err("fail invalid buf handle\n");
>> +		return;
>> +	}
>> +
>> +	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);
> 
> Variable declaration mixed with code, move the variable declaration to the top.
Will fix in the next patch>
>> +
>> +	dev_dbg(buf->dev, "detach dmabuf of isp user bo 0x%llx size %ld",
>> +		buf->gpu_addr, buf->size);
>> +
>> +	if (buf->vaddr)
>> +		dma_buf_vunmap_unlocked(buf->dbuf, &map);
>> +
>> +	// put dmabuf for exported ones
>> +	dma_buf_put(buf->dbuf);
> 
> The dmabuf shouldn't be put from the detach_dmabuf memop. Remove this.
> 
Will fix in the next patch>> +
>> +	kfree(buf);
>> +}
> 
> [snip]
> 
>> +static void isp4vid_vb2_dmabuf_ops_release(struct dma_buf *dbuf)
>> +{
>> +	struct isp4vid_vb2_buf *buf = dbuf->priv;
>> +
>> +	/* drop reference obtained in vb2_isp4vid_get_dmabuf */
> 
> s/vb2_isp4vid_get_dmabuf/isp4vid_vb2_get_dmabuf/
> 
Will fix in the next patch>> +	if (buf->is_expbuf)
>> +		isp4vid_vb2_put(dbuf->priv);
>> +	else
>> +		dev_dbg(buf->dev, "ignore buf release for implicit case");
>> +}
> 
> [snip]
> 
>> +static struct dma_buf *isp4vid_vb2_get_dmabuf(struct vb2_buffer *vb,
>> +					      void *buf_priv,
>> +					      unsigned long flags)
>> +{
>> +	struct isp4vid_vb2_buf *buf = buf_priv;
>> +	struct dma_buf *dbuf;
>> +
>> +	if (buf->dbuf) {
>> +		dev_dbg(buf->dev,
>> +			"dbuf already created, reuse implicit dbuf\n");
>> +		dbuf = buf->dbuf;
> 
> The dmabuf is reused here without taking a reference to it. When the get_dmabuf
> memop is called by vb2_core_expbuf(), it assumes that a reference to the dmabuf
> is acquired. So you need to add `get_dma_buf(dbuf)` here.
After test, we found we can't add get_dma_buf(dbuf) here because it will 
make cheese APP fail to open camera with following error:
amdgpu: [drm] *ERROR* failed to alloc gart kernel buffer (-28)>
>> +	} else {
>> +		dbuf = isp4vid_get_dmabuf(vb, buf_priv, flags);
>> +		dev_dbg(buf->dev, "created new dbuf\n");
>> +	}
>> +	buf->is_expbuf = true;
>> +	refcount_inc(&buf->refcount);
>> +
>> +	dev_dbg(buf->dev, "buf exported, refcount %d\n",
>> +		buf->refcount.refs.counter);
>> +
>> +	return dbuf;
>> +}
> 
> [snip]
> 
>> +static void *isp4vid_vb2_get_userptr(struct vb2_buffer *vb, struct device *dev,
>> +				     unsigned long vaddr, unsigned long size)
>> +{
>> +	struct isp4vid_vb2_buf *buf;
>> +	struct frame_vector *vec;
>> +	int n_pages, offset, i;
>> +	int ret = -ENOMEM;
>> +
>> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>> +	if (!buf)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	buf->dev = dev;
>> +	buf->dma_dir = vb->vb2_queue->dma_dir;
>> +	offset = vaddr & ~PAGE_MASK;
>> +	buf->size = size;
>> +	vec = vb2_create_framevec(vaddr, size,
>> +				  buf->dma_dir == DMA_FROM_DEVICE ||
>> +				  buf->dma_dir == DMA_BIDIRECTIONAL);
>> +	if (IS_ERR(vec)) {
>> +		kfree(buf);
>> +		return vec;
>> +	}
> 
> You can combine the error handling here with the error path at the bottom of the
> function instead of duplicating the `kfree(buf)`.
> 
Will fix in the next patch>> +	buf->vec = vec;
>> +	n_pages = frame_vector_count(vec);
>> +	if (frame_vector_to_pages(vec) < 0) {
>> +		unsigned long *nums = frame_vector_pfns(vec);
>> +
>> +		/*
>> +		 * We cannot get page pointers for these pfns. Check memory is
>> +		 * physically contiguous and use direct mapping.
>> +		 */
>> +		for (i = 1; i < n_pages; i++)
>> +			if (nums[i - 1] + 1 != nums[i])
>> +				goto err_destroy_free;
>> +		buf->vaddr = (__force void *)
>> +			     ioremap(__pfn_to_phys(nums[0]), size + offset);
>> +	} else {
>> +		buf->vaddr = vm_map_ram(frame_vector_pages(vec), n_pages, -1);
>> +	}
>> +
>> +	if (!buf->vaddr)
>> +		goto err_destroy_free;
>> +
>> +	buf->vaddr = ((char *)buf->vaddr) + offset;
>> +	return buf;
>> +
>> +err_destroy_free:
>> +	vb2_destroy_framevec(vec);
>> +	kfree(buf);
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +static void isp4vid_vb2_put(void *buf_priv)
>> +{
>> +	struct isp4vid_vb2_buf *buf = (struct isp4vid_vb2_buf *)buf_priv;
>> +	struct amdgpu_bo *bo = (struct amdgpu_bo *)buf->bo;
>> +
>> +	dev_dbg(buf->dev,
>> +		"release isp user bo 0x%llx size %ld refcount %d is_expbuf %d",
>> +		buf->gpu_addr, buf->size,
>> +		buf->refcount.refs.counter, buf->is_expbuf);
>> +
>> +	if (refcount_dec_and_test(&buf->refcount)) {
>> +		amdgpu_bo_free_isp_user(bo);
>> +
>> +		// put implicit dmabuf here, detach_dmabuf will not be called
> 
> Comment style (use /**/ instead of //).
> 
Will fix in the next patch>> +		if (!buf->is_expbuf)
>> +			dma_buf_put(buf->dbuf);
>> +
>> +		vfree(buf->vaddr);
>> +		kfree(buf);
>> +		buf = NULL;
> 
> `buf = NULL;` here is superfluous; you can remove it.
> 
Will fix in the next patch>> +	} else {
>> +		dev_warn(buf->dev, "ignore buffer free, refcount %u > 0",
>> +			 refcount_read(&buf->refcount));
> 
> This refcount_read() is a possible use-after-free because `buf` is accessed
> after isp4vid_vb2_put() puts its reference to `buf`. So something else could put
> the last reference to `buf` and free it after this refcount dec but before the
> refcount_read(). Maybe just remove this dev_warn() entirely?
> 
The warning is important to debug mem related issue, plan to keep it but 
without accessing buf or buf->refcount here. Do you think it 
acceptible?>> +	}
>> +}
>> +
>> +static void *isp4vid_vb2_alloc(struct vb2_buffer *vb, struct device *dev,
>> +			       unsigned long size)
>> +{
>> +	struct isp4vid_dev *isp_vdev = vb2_get_drv_priv(vb->vb2_queue);
>> +	struct isp4vid_vb2_buf *buf = NULL;
>> +	struct amdgpu_bo *bo;
>> +	u64 gpu_addr;
>> +	u32 ret;
>> +
>> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL | vb->vb2_queue->gfp_flags);
>> +	if (!buf)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	buf->dev = dev;
>> +	buf->size = size;
>> +	buf->vaddr = vmalloc_user(buf->size);
>> +	if (!buf->vaddr) {
>> +		kfree(buf);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	buf->dma_dir = vb->vb2_queue->dma_dir;
>> +	buf->handler.refcount = &buf->refcount;
>> +	buf->handler.put = isp4vid_vb2_put;
>> +	buf->handler.arg = buf;
> 
> What is buf->handler for? I don't see it used anywhere in the entire patchset; I
> can delete `handler` from `struct isp4vid_vb2_buf` along with these lines and it
> compiles.
> 
Yes, will remove it.>> +
>> +	// get implicit dmabuf
> 
> Comment style.
> 
>> +	buf->dbuf = isp4vid_get_dmabuf(vb, buf, 0);
>> +	if (!buf->dbuf) {
>> +		dev_err(dev, "fail to get dmabuf\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
> 
> Doesn't free `buf` or `buf->vaddr` on error here. Also, comment style.
> 
Will fix in the next patch>> +
>> +	// create isp user BO and obtain gpu_addr
> 
> Comment style.
> 
Will fix in the next patch>> +	ret = 
amdgpu_bo_create_isp_user(isp_vdev->amdgpu_dev, buf->dbuf,
>> +					AMDGPU_GEM_DOMAIN_GTT, &bo, &gpu_addr);
>> +	if (ret) {
>> +		dev_err(dev, "fail to create BO\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
> 
> Doesn't free `buf` or `buf->vaddr` or put `buf->dbuf` on error here.
> 
Will fix in the next patch>> +
>> +	buf->bo = (void *)bo;
>> +	buf->gpu_addr = gpu_addr;
>> +
>> +	refcount_set(&buf->refcount, 1);
> 
> This discards the refcount inc triggered from amdgpu_bo_create_isp_user() when
> it calls get_dma_buf(), leading to a use-after-free. Move this refcount_set()
> up, preferably right after vmalloc_user() or right after `buf` is allocated so
> there's no risk of this issue occurring again in the future.
> 
Ignore it as you did correction in another mail.>> +
>> +	dev_dbg(dev, "allocated isp user bo 0x%llx size %ld refcount %d",
>> +		buf->gpu_addr, buf->size, buf->refcount.refs.counter);
>> +
>> +	return buf;
>> +}
> 
> [snip]
> 
> Sultan