| Message ID | 20241203-udmabuf-fixes-v1-3-f99281c345aa@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fixes for udmabuf (memfd sealing checks and a leak) | expand |
Hi Jann, > 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@google.com> > --- > drivers/dma-buf/udmabuf.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index > 8ce77f5837d71a73be677cad014e05f29706057d..aae0071be14a2c83a428b59 > ea9e905c7173232be 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -317,12 +317,11 @@ static int check_memfd_seals(struct file *memfd) > return 0; > } > > -static int export_udmabuf(struct udmabuf *ubuf, > - struct miscdevice *device, > - u32 flags) > +static struct dma_buf *export_udmabuf(struct udmabuf *ubuf, > + struct miscdevice *device, > + u32 flags) > { > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > - struct dma_buf *buf; > > ubuf->device = device; > exp_info.ops = &udmabuf_ops; > @@ -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. > + 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(). With that, Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > + > + ret = dma_buf_fd(dmabuf, flags); > + if (ret < 0) > + dma_buf_put(dmabuf); > > kvfree(folios); > return ret; > > -- > 2.47.0.338.g60cca15819-goog
On Wed, Dec 4, 2024 at 10:14 AM Kasireddy, Vivek <vivek.kasireddy@intel.com> 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@google.com> [...] > > @@ -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@intel.com> Thanks!
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 8ce77f5837d71a73be677cad014e05f29706057d..aae0071be14a2c83a428b59ea9e905c7173232be 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -317,12 +317,11 @@ static int check_memfd_seals(struct file *memfd) return 0; } -static int export_udmabuf(struct udmabuf *ubuf, - struct miscdevice *device, - u32 flags) +static struct dma_buf *export_udmabuf(struct udmabuf *ubuf, + struct miscdevice *device, + u32 flags) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info); - struct dma_buf *buf; ubuf->device = device; exp_info.ops = &udmabuf_ops; @@ -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); + 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 */ + + ret = dma_buf_fd(dmabuf, flags); + if (ret < 0) + dma_buf_put(dmabuf); kvfree(folios); return ret;
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@google.com> --- drivers/dma-buf/udmabuf.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)