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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] dma-buf: deadlock due to synchronous wait for I/O in invalidate_mappings
- [Medium] dma-buf: memory leak in dma_buf_io_ctx_create() on invalid dev_ops
- [Medium] dma-buf: memory leak of dma_buf_io_map in __dma_buf_io_create_map()
- [Medium] dma-buf: memory leak in dma_buf_io_ctx_release() due to early return
--

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

dma-buf: introduce initial file I/O infrastructure

This commit introduces basic building blocks (dma_buf_io_ctx and
dma_buf_io_map) to natively use dma-buf in the read-write/IO path.
It provides the initial API for drivers and upper layer subsystems
to interact with the buffers.

> 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

[ ... ]

> +static void dma_buf_io_kill_map(struct dma_buf_io_ctx *ctx)
> +{
> +	struct dma_buf_io_map *map;
> +
> +	dma_resv_assert_held(ctx->dmabuf->resv);
> +
> +	map = rcu_dereference_protected(ctx->map,
> +				dma_resv_held(ctx->dmabuf->resv));
> +	if (!map)
> +		return;
> +
> +	rcu_assign_pointer(ctx->map, NULL);
> +	percpu_ref_kill(&map->refs);
> +	/* make sure the map is not visible via ctx->map */
> +	synchronize_rcu_expedited();
> +	wait_for_completion(&map->drained);

[Severity: High]
Does this synchronously block on I/O completion while holding the
dma_resv lock?

dma_buf_io_invalidate_mappings() is intended as an invalidation callback
which is invoked while holding the dma_resv lock to evict or move the
dma-buf.

The dynamic importer contract (include/linux/dma-buf.h) states that
importers should set fences for any access they can't disable immediately
rather than blocking indefinitely. Since block I/O can be delayed or depend
on memory allocation, waiting for it in the memory reclaim/eviction path
could potentially cause a deadlock.

> +	dma_buf_io_map_release(map);
> +}

[ ... ]

> +static struct dma_buf_io_map *__dma_buf_io_create_map(struct dma_buf_io_ctx *ctx)
> +{
> +	struct dma_buf *dmabuf = ctx->dmabuf;
> +	struct dma_buf_io_map *map;
> +	long ret;
> +
> +	dma_resv_assert_held(dmabuf->resv);
> +
> +	if (ctx->is_dead)
> +		return ERR_PTR(-ENOENT);
> +
> +	map = __dma_buf_io_get_map(ctx);
> +	if (map)
> +		return map;
> +
> +	map = ctx->dev_ops->map(ctx);
> +	if (IS_ERR(map))
> +		return map;
> +
> +	if (WARN_ON_ONCE(!map->min_seg_shift))
> +		return ERR_PTR(-EFAULT);

[Severity: Medium]
If this min_seg_shift check fails, does it leak the newly allocated map
structure?

It appears to return ERR_PTR(-EFAULT) directly without invoking
dma_buf_io_map_release(map) or the driver's unmap callback.

> +
> +	ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL,
> +				    true, MAX_SCHEDULE_TIMEOUT);

[ ... ]

> +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)
> +{
> +	struct dma_buf_io_ctx *ctx;
> +	int ret;
> +
> +	if (!file->f_op->init_dma_buf_io_ctx)
> +		return -EOPNOTSUPP;
> +
> +	ctx = kzalloc_obj(*ctx);
> +	if (!ctx)
> +		return -ENOMEM;
> +	ctx->dir = dir;
> +	ctx->dmabuf = dmabuf;
> +	get_dma_buf(dmabuf);
> +
> +	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]
Could this error path leak the context and the dma-buf reference?

The early return bypasses kfree(ctx) and dma_buf_put(dmabuf). It also
doesn't seem to provide a way for the driver to clean up any internal
resources it might have allocated during the init_dma_buf_io_ctx() call.

> +
> +	*out_ctx = ctx;
> +	return 0;
> +}

[ ... ]

> +void dma_buf_io_ctx_release(struct dma_buf_io_ctx *ctx)
> +{
> +	dma_buf_io_ctx_shutdown(ctx);
> +
> +	if (WARN_ON_ONCE(rcu_dereference_protected(ctx->map, true)))
> +		return;

[Severity: Medium]
Could this early return cause a memory leak?

If the warning triggers, the function returns immediately and skips the
final teardown, meaning ctx->dev_ops->release(ctx), dma_buf_put(ctx->dmabuf),
and kfree(ctx) won't be executed.

> +
> +	ctx->dev_ops->release(ctx);
> +	dma_buf_put(ctx->dmabuf);
> +	kfree(ctx);
> +}

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