Re: [PATCH 3/3] udmabuf: fix memory leak on last export_udmabuf() error path

[Jann Horn]

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!