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

Du, Bin
2025-07-25T09:22:52Z

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