On Wed, Dec 4, 2024 at 10:14 AM Kasireddy, Vivek
<vivek.kasireddy@xxxxxxxxx> wrote:
> > Subject: [PATCH 3/3] udmabuf: fix memory leak on last export_udmabuf()
> > error path
> >
> > In export_udmabuf(), if dma_buf_fd() fails because the FD table is full, a
> > dma_buf owning the udmabuf has already been created; but the error
> > handling
> > in udmabuf_create() will tear down the udmabuf without doing anything
> > about
> > the containing dma_buf.
> >
> > This leaves a dma_buf in memory that contains a dangling pointer; though
> > that doesn't seem to lead to anything bad except a memory leak.
> >
> > Fix it by moving the dma_buf_fd() call out of export_udmabuf() so that we
> > can give it different error handling.
> >
> > Note that the shape of this code changed a lot in commit 5e72b2b41a21
> > ("udmabuf: convert udmabuf driver to use folios"); but the memory leak
> > seems to have existed since the introduction of udmabuf.
> >
> > Fixes: fbb0de795078 ("Add udmabuf misc device")
> > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
[...]
> > @@ -330,11 +329,7 @@ static int export_udmabuf(struct udmabuf *ubuf,
> > exp_info.priv = ubuf;
> > exp_info.flags = O_RDWR;
> >
> > - buf = dma_buf_export(&exp_info);
> > - if (IS_ERR(buf))
> > - return PTR_ERR(buf);
> > -
> > - return dma_buf_fd(buf, flags);
> flags is now unused in this function.
Ack, will remove in v2.
> > + return dma_buf_export(&exp_info);
> > }
> >
> > static long udmabuf_pin_folios(struct udmabuf *ubuf, struct file *memfd,
> > @@ -391,6 +386,7 @@ static long udmabuf_create(struct miscdevice
> > *device,
> > struct folio **folios = NULL;
> > pgoff_t pgcnt = 0, pglimit;
> > struct udmabuf *ubuf;
> > + struct dma_buf *dmabuf;
> > long ret = -EINVAL;
> > u32 i, flags;
> >
> > @@ -451,9 +447,16 @@ static long udmabuf_create(struct miscdevice
> > *device,
> > }
> >
> > flags = head->flags & UDMABUF_FLAGS_CLOEXEC ? O_CLOEXEC : 0;
> > - ret = export_udmabuf(ubuf, device, flags);
> > - if (ret < 0)
> > + dmabuf = export_udmabuf(ubuf, device, flags);
> > + if (IS_ERR(dmabuf)) {
> > + ret = PTR_ERR(dmabuf);
> > goto err;
> > + }
> > + /* ownership of ubuf is held by the dmabuf from here */
> Please also add a comment here that says that if dma_buf_fd() fails,
> then calling dma_buf_put() will enable cleanup to be done via release().
Ack, added in v2.
> With that,
>
> Acked-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
Thanks!
