Re: [PATCH v7 01/13] dma-buf: introduce initial file I/O infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma-buf: map leak and deadlock on invalid min_seg_shift
- [Medium] dma-buf: memory leak in dma_buf_io_ctx_create() dev_ops validation
--

commit cebbd64805f3eac146806ef3dc1344fdf0a76c98
Author: Pavel Begunkov <asml.silence@gmail.com>

dma-buf: introduce initial file I/O infrastructure

This patch adds basic building blocks serving as an API between drivers and
upper layer subsystems to natively use dma-buf in the IO paths. It introduces
the dma_buf_io_ctx and dma_buf_io_map structures to handle mapping, unmapping,
and invalidation operations.

> diff --git a/drivers/dma-buf/dma-buf-io.c b/drivers/dma-buf/dma-buf-io.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/dma-buf/dma-buf-io.c
[ ... ]
> +struct dma_buf_io_map *dma_buf_io_create_map(struct dma_buf_io_ctx *ctx)
> +{
[ ... ]
> +	map = ctx->dev_ops->map(ctx);
> +	if (!IS_ERR(map)) {
> +		atomic_inc(&ctx->active_maps);
> +		atomic_inc(&ctx->all_maps);
> +	}
> +	dma_resv_unlock(dmabuf->resv);
> +
> +	if (IS_ERR(map))
> +		return map;
> +	if (WARN_ON_ONCE(!map->min_seg_shift))
> +		return ERR_PTR(-EFAULT);

[Severity: High]
Does this error path leak the map and deadlock the system?

If map->min_seg_shift evaluates to 0 (for example, if an odd-length DMA
segment causes __ffs() to return 0), the WARN_ON_ONCE check will trigger.

Since atomic_inc(&ctx->active_maps) and atomic_inc(&ctx->all_maps) have
already executed for a valid map, returning ERR_PTR(-EFAULT) here skips
dropping the map's percpu_ref count and leaves the atomic counters
permanently elevated. 

Could subsequent cleanup calls to dma_buf_io_wait_active_maps() or
dma_buf_io_wait_maps() then block forever waiting for the counters to reach
zero?

[ ... ]
> +int dma_buf_io_ctx_create(struct file *file,
> +			   struct dma_buf *dmabuf,
> +			   enum dma_data_direction dir,
> +			   struct dma_buf_io_ctx **out_ctx)
> +{
[ ... ]
> +	ret = file->f_op->init_dma_buf_io_ctx(file, ctx);
> +	if (ret) {
> +		kfree(ctx);
> +		dma_buf_put(dmabuf);
> +		return ret;
> +	}
> +
> +	if (WARN_ON_ONCE(!ctx->dev_ops ||
> +			 !ctx->dev_ops->map ||
> +			 !ctx->dev_ops->unmap ||
> +			 !ctx->dev_ops->release))
> +		return -EINVAL;

[Severity: Medium]
Can this early return leak the context and the dma-buf reference?

If the init_dma_buf_io_ctx callback succeeds but fails to populate the
required dev_ops, this path returns -EINVAL immediately.

It appears to skip the kfree(ctx) and dma_buf_put(dmabuf) teardown
operations that are present in the preceding error handling block.

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