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

Sultan Alsawaf
2025-07-23T17:55:52Z

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.

> +
> + 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.

> +
> + 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/

> + 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.

> + } 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)`.

> + 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 //).

> + 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.

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

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

> +
> + // 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.

> +
> + // create isp user BO and obtain gpu_addr

Comment style.

> + 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.

> +
> + 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.

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