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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] io_uring/rw: dmabuf_map leaked on page waitqueue retry and cancellation
--

commit 671f0754c33400252fbbb2aab640bc25813b29e5
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.h b/io_uring/dma-buf.h
> new file mode 100644
> index 0000000000000..a876a9e5e272b
> --- /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)
> +{
> +	bool nowait = issue_flags & IO_URING_F_NONBLOCK;
> +	struct dma_buf_io_map *map;
> +	struct io_buf_dma *bd;
> +
> +	if (!IS_ENABLED(CONFIG_DMA_SHARED_BUFFER))
> +		return -EOPNOTSUPP;
> +	if (!(req->flags & REQ_F_DMABUF))
> +		return 0;
> +
> +	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;

[Severity: High]
If a request is re-issued (for example, after returning -EIOCBQUEUED and being
woken up by the waitqueue), io_init_rw_fixed() is called again and 
unconditionally invokes io_attach_dmabuf_map(). 

Does this overwrite req->dmabuf_map with a new reference without dropping the
old one, causing a memory leak of the dma_buf_io_map reference?

> +	iter->dmabuf_map = map;
> +	return 0;
> +}
[ ... ]
> 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
[ ... ]
> @@ -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;

[Severity: High]
When io_read() arms a page waitqueue (e.g., via io_rw_should_retry()) and
returns -EIOCBQUEUED, the dmabuf map reference is deliberately not dropped here.

If the request is later cancelled while on the waitqueue, does this cause a
memory leak? The cleanup handler invoked on cancellation via io_clean_op()
(which is io_readv_writev_cleanup() for IORING_OP_READ_FIXED) doesn't appear
to clean up req->dmabuf_map.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791975048.git.asml.silence@gmail.com?part=13