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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] dma-buf: memory leak in dma_buf_io_ctx_create() on dev_ops validation failure
- [High] dma-buf: GFP_KERNEL allocation while holding dma_resv lock causes reclaim deadlocks
- [High] dma-buf: deadlock when waiting for I/O completion with dma_resv lock held
- [Medium] dma-buf: memory leak of map object in __dma_buf_io_create_map() on min_seg_shift failure
- [Medium] dma-buf: memory leak when ctx->dev_ops validation fails
- [Medium] dma-buf: map and percpu_ref leak when min_seg_shift is zero
--

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

dma-buf: introduce initial file I/O infrastructure

This commit introduces the basic building blocks (dma_buf_io_ctx and
dma_buf_io_map) to support using dma-buf natively in read-write / IO paths.
It provides an API between upper layer subsystems (like io_uring) and importer
drivers to facilitate map creation and invalidation.

> diff --git a/drivers/dma-buf/dma-buf-io.c b/drivers/dma-buf/dma-buf-io.c
> new file mode 100644
> index 0000000000000..9ba9c17f900af
> --- /dev/null
> +++ b/drivers/dma-buf/dma-buf-io.c
> @@ -0,0 +1,183 @@
[ ... ]
> +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);
[ ... ]
> +	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 waiting for I/O completion here risk a deadlock with memory reclaim?

Since dma_buf_io_kill_map() is called with the dma_resv lock held (and
asserted above), waiting synchronously for unbounded in-flight I/O via
wait_for_completion() could block memory shrinkers. 

If the active I/O operation itself requires memory allocation, it creates an
ABBA deadlock where the I/O needs memory, the shrinker needs the dma_resv
lock, and this function holds the lock while waiting for the I/O.

This is especially problematic when called through
dma_buf_io_invalidate_mappings() from an exporter's invalidate_mappings
callback during memory pressure.

> +	dma_buf_io_map_release(map);
> +}
> +
> +int dma_buf_io_init_map(struct dma_buf_io_ctx *ctx, struct dma_buf_io_map *map,
> +			struct sg_table *sgt)
> +{
> +	unsigned seg_shift = ~0U;
> +	struct scatterlist *sg;
> +	unsigned long tmp;
> +	int ret;
> +
> +	for_each_sgtable_dma_sg(sgt, sg, tmp)
> +		seg_shift = min(seg_shift, __ffs(sg_dma_len(sg)));
> +
> +	ret = percpu_ref_init(&map->refs, dma_buf_io_map_refs_cb, 0, GFP_KERNEL);

[Severity: High]
Can this GFP_KERNEL allocation trigger a reclaim deadlock?

dma_buf_io_init_map() initializes the percpu_ref using GFP_KERNEL, which
permits direct memory reclaim. This function is invoked from the driver's
map callback via __dma_buf_io_create_map().

However, the caller dma_buf_io_create_map() acquires the dma_resv lock
before executing this sequence:

dma_buf_io_create_map() {
	ret = dma_resv_lock_interruptible(ctx->dmabuf->resv, NULL);
	...
	map = __dma_buf_io_create_map(ctx);
}

Because GPU and other dma-buf shrinkers attempt to acquire dma_resv locks
to evict buffers, doing a GFP_KERNEL allocation while holding this lock
creates an ABBA deadlock vulnerability. Should this use GFP_NOWAIT or
be moved outside the lock?

> +	if (ret)
> +		return ret;
[ ... ]
> +static 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))
> +		return map;
> +
> +	if (WARN_ON_ONCE(!map->min_seg_shift))
> +		return ERR_PTR(-EFAULT);

[Severity: Medium]
If map->min_seg_shift is zero, does this leak the map object?

The early return -EFAULT happens after the driver successfully allocates
and initializes the map via ctx->dev_ops->map(ctx). Returning here bypasses
releasing the map and its percpu_ref initialized in dma_buf_io_init_map(),
as well as any driver-specific resources.

> +
> +	ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL,
[ ... ]
> +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: High]
If the dev_ops validation fails here, do we leak ctx and the dmabuf
reference?

This early return abandons the ctx object and dmabuf reference without
cleaning them up. It skips freeing ctx, dropping the dmabuf reference
acquired earlier by get_dma_buf(dmabuf), and executing any driver-specific
cleanup that might have occurred during init_dma_buf_io_ctx().

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

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