Re: [PATCH v10 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: memory and reference leak in dma_buf_io_ctx_create missing dev_ops
- [High] dma-buf: map leaked on invalid min_seg_shift in __dma_buf_io_create_map
- [High] dma-buf: deadlock risk by blocking on I/O inside dma_resv lock
- [Medium] dma-buf: resource leak in dma_buf_io_ctx_release() on map check failure
--

commit 420452d61f9f92ad48f07cea0dcd04c4e4a65bac
Author: Pavel Begunkov <asml.silence@gmail.com>
Subject: dma-buf: introduce initial file I/O infrastructure

This commit introduces the initial file I/O infrastructure for natively using
dma-buf in the read-write path. It adds building blocks like dma_buf_io_ctx
and dma_buf_io_map to serve as an interface between upper layer subsystems
and the importer driver. Operations like mapping and unmapping are delegated
to the importer driver via dma_buf_io_ops.

> 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 block indefinitely on hardware I/O completion while holding the
dmabuf->resv lock?

dma_buf_io_kill_map() is called from the invalidate_mappings callback. If the
exporter calls invalidate_mappings while holding the dma_resv lock during memory
reclaim, waiting synchronously for all in-flight I/O requests to drop their
references could cause an ABBA deadlock. Memory reclaim might try to acquire
the same dma_resv lock.

Additionally, dynamic importer rules dictate that invalidate_mappings must not
block indefinitely, but should instead detach or set fences so the exporter can
wait on them.

> +	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: High]
Does this error path leak the map structure?

When map->min_seg_shift is zero, the function returns ERR_PTR directly. It
looks like this skips invoking dma_buf_io_map_release(map) or
ctx->dev_ops->unmap(ctx, map), which might permanently leak the map structure,
its initialized percpu_ref, and any associated hardware DMA mappings.

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

[Severity: High]
Can this wait cause a deadlock by blocking other lockers like shrinkers?

Since __dma_buf_io_create_map() holds the dma_resv lock, synchronously waiting
on dma_resv_wait_timeout() with MAX_SCHEDULE_TIMEOUT might block any other
dma_resv locker while waiting for hardware fences to signal.

> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -EAGAIN;
> +		dma_buf_io_map_release(map);
> +		return ERR_PTR(ret);
> +	}
> +
> +	/* get a reference for the caller */
> +	percpu_ref_get(&map->refs);
> +	rcu_assign_pointer(ctx->map, map);
> +	return map;
> +}

[ ... ]

> +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: High]
Does returning directly here leak the context and the dma_buf reference?

After allocating the context and getting a reference to dmabuf, if any of the
required device operations are missing, the function returns -EINVAL directly.
This appears to leave the allocated ctx unfreed and the dmabuf reference
unreleased, without calling any cleanup like kfree() or dma_buf_put(), nor any
driver-specific release.

> +
> +	*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]
Does returning early from the destructor leak the context and pin the dma_buf?

If the ctx->map is still populated during release, returning early here skips
the final ctx->dev_ops->release(ctx), dma_buf_put(ctx->dmabuf), and
kfree(ctx). Teardown functions shouldn't silently leak resources when they
encounter an invalid state.

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

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