Re: [PATCH v9 13/13] io_uring/rsrc: add dmabuf backed registered buffers

[sashiko-bot@kernel.org]

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 <asml.silence@gmail.com>

    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/cover.1791336930.git.asml.silence@gmail.com?part=13