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

[Sultan Alsawaf <sultan@kerneltoast.com>]

On Fri, Jul 25, 2025 at 05:22:41PM +0800, Du, Bin wrote:
> > > +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)

I see, it's because buf->is_expbuf is set to true even for the implicit dbuf, so
the initial reference on the implicit dbuf is never put, causing a leak.

Also, the refcount increment in isp4vid_vb2_get_dmabuf() is done every time even
when reusing the existing dbuf, but releasing the dbuf will only do a single
refcount decrement. This also causes a leak.

And, isp4vid_get_dmabuf() may fail but isp4vid_vb2_get_dmabuf() doesn't check
the return value, so there may be another leak when isp4vid_get_dmabuf() fails
because of the refcount increment. The refcount increment and setting of
buf->is_expbuf to true should only be done on success.

I have fixed all of these isp4vid_vb2_get_dmabuf() issues in the following diff,
please try it:

--- a/drivers/media/platform/amd/isp4/isp4_video.c
+++ b/drivers/media/platform/amd/isp4/isp4_video.c
@@ -476,18 +476,22 @@ static struct dma_buf *isp4vid_vb2_get_dmabuf(struct vb2_buffer *vb,
 					      unsigned long flags)
 {
 	struct isp4vid_vb2_buf *buf = buf_priv;
-	struct dma_buf *dbuf;
+	struct dma_buf *dbuf = buf->dbuf;
 
-	if (buf->dbuf) {
+	if (dbuf) {
 		dev_dbg(buf->dev,
-			"dbuf already created, reuse implicit dbuf\n");
-		dbuf = buf->dbuf;
+			"dbuf already created, reuse %s dbuf\n",
+			buf->is_expbuf ? "exported" : "implicit");
+		get_dma_buf(dbuf);
 	} else {
 		dbuf = isp4vid_get_dmabuf(vb, buf_priv, flags);
+		if (!dbuf)
+			return NULL;
+
 		dev_dbg(buf->dev, "created new dbuf\n");
+		buf->is_expbuf = true;
+		refcount_inc(&buf->refcount);
 	}
-	buf->is_expbuf = true;
-	refcount_inc(&buf->refcount);
 
 	dev_dbg(buf->dev, "buf exported, refcount %d\n",
 		buf->refcount.refs.counter);
--

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

Yes, that sounds good. So something like this:
`dev_warn(buf->dev, "ignore buffer free, refcount > 0");`

Sultan