Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] io_uring/rw: dmabuf_map memory leak upon request reissue overwriting req->dmabuf_map - [High] io_uring/rsrc: dmabuf_map leak via cloning dmabuf to IOPOLL ring bypassing restrictions --
commit 6cf565430e55d29c9af3badb73c4c8775d233005 Author: Pavel Begunkov <[email protected]> io_uring/rsrc: add dmabuf backed registered buffers Implement dmabuf backed registered buffers. To register them, the user should specify IO_REGBUF_TYPE_DMABUF for the regitration and pass the desired dmabuf fd and a file for which it should be registered. > diff --git a/io_uring/dma-buf.c b/io_uring/dma-buf.c > index 0000000000000..f40c9d87ed8aa > --- /dev/null > +++ b/io_uring/dma-buf.c [ ... ] > +int io_register_dmabuf(struct io_ring_ctx *ctx, > + struct io_uring_regbuf_desc *desc, > + struct io_mapped_ubuf *imu) > +{ [ ... ] > + if (ctx->flags & IORING_SETUP_IOPOLL) > + return -EOPNOTSUPP; [ ... ] > + bd->target_file = target_file; > + imu->len = dmabuf->size; > + imu->release = io_release_dmabuf; > + imu->priv = bd; > + imu->flags = IO_REGBUF_F_DMABUF; > + imu->dir = IO_BUF_DEST | IO_BUF_SOURCE; > + dma_buf_put(dmabuf); > + return 0; [Severity: High] Should IO_REGBUF_F_UNCLONEABLE be included in imu->flags here to prevent IOPOLL rings from cloning this buffer? While io_register_dmabuf() explicitly rejects rings with IORING_SETUP_IOPOLL set during initial registration, omitting the uncloneable flag allows an IOPOLL ring to bypass this restriction by cloning the dmabuf via io_clone_buffers(). If the buffer is cloned to an IOPOLL ring, polled I/O requests will finish via io_complete_rw_iopoll(), which does not call io_detach_dmabuf_map(req) (unlike io_complete_rw). Does this result in a permanent leak of the dma_buf_io_map reference for any async polled request? > diff --git a/io_uring/dma-buf.h b/io_uring/dma-buf.h > index 0000000000000..10d2efdd8c9f1 > --- /dev/null > +++ b/io_uring/dma-buf.h [ ... ] > +static inline int io_attach_dmabuf_map(struct io_kiocb *req, > + struct iov_iter *iter, > + unsigned issue_flags) > +{ [ ... ] > + bd = req->buf_node->buf->priv; > + map = dma_buf_io_get_map(bd->ctx, nowait); > + if (unlikely(IS_ERR(map))) > + return PTR_ERR(map); > + req->dmabuf_map = map; > + iter->dmabuf_map = map; > + return 0; > +} [Severity: High] Could this overwrite and leak the previous req->dmabuf_map if the request is reissued? When a read or write request encounters a reissue condition, it returns -EIOCBQUEUED. In io_read_fixed() and io_write_fixed(), this causes it to skip calling io_detach_dmabuf_map(). When the request is subsequently reissued, it executes io_init_rw_fixed() again, which unconditionally calls io_attach_dmabuf_map(). This fetches a new map and overwrites req->dmabuf_map without dropping the previous reference. Would it make sense to check if req->dmabuf_map is already populated before requesting and assigning a new map, or to ensure it is detached before reassignment? > diff --git a/io_uring/rw.c b/io_uring/rw.c > index 432820f862514..ba0e3e7de557d 100644 > --- a/io_uring/rw.c > +++ b/io_uring/rw.c [ ... ] > @@ -363,13 +364,18 @@ static int io_init_rw_fixed(struct io_kiocb *req, > unsigned int issue_flags, > struct io_async_rw *io = req->async_data; > int ret; > > - if (io->bytes_done) > - return 0; > + if (!io->bytes_done) { > + ret = __io_import_reg_buf(req, &io->iter, rw->addr, rw->len, > ddir, > + issue_flags, > IO_REGBUF_IMPORT_ALLOW_DMABUF); > + if (ret) > + return ret; > + iov_iter_save_state(&io->iter, &io->iter_state); > + } > > - ret = io_import_reg_buf(req, &io->iter, rw->addr, rw->len, ddir, > - issue_flags); > - iov_iter_save_state(&io->iter, &io->iter_state); > - return ret; > + ret = io_attach_dmabuf_map(req, &io->iter, issue_flags); > + if (ret) > + return ret; > + return 0; > } [ ... ] > @@ -1231,7 +1239,10 @@ int io_read_fixed(struct io_kiocb *req, unsigned int > issue_flags) > if (unlikely(ret)) > return ret; > > - return io_read(req, issue_flags); > + ret = io_read(req, issue_flags); > + if (ret != -EIOCBQUEUED) > + io_detach_dmabuf_map(req); > + return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
