Re: [PATCH v6 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:
- [Critical] dma-buf: TOCTOU race allows a stale map to escape concurrent invalidation
- [High] dma-buf: Use-After-Free of ctx in waitqueue due to race condition
- [High] dma-buf: GFP_KERNEL allocation performed while holding dmabuf->resv ww_mutex
- [Medium] dma-buf: Hardcoded kfree(map) restricts driver structure embedding
- [Medium] dma-buf: Memory leak of ctx and dmabuf reference on incomplete dev_ops error path
- [Medium] dma-buf: Memory leak of map structure and DMA mappings on min_seg_shift validation error
--

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

This commit adds basic building blocks serving as a glue and API between
drivers and upper layer subsystems providing the uAPI. It introduces
struct dma_buf_io_ctx and struct dma_buf_io_map, allowing dma-buf to be
natively used in the read-write / IO path.

> diff --git a/drivers/dma-buf/dma-buf-io.c b/drivers/dma-buf/dma-buf-io.c
> new file mode 100644
> index 0000000000000..8312637a299f6
> --- /dev/null
> +++ b/drivers/dma-buf/dma-buf-io.c
[ ... ]
> @@ -22,17 +22,17 @@ static void dma_buf_io_map_release_work(struct work_struct *work)
>  {
>  	struct dma_buf_io_map *map = container_of(work, struct dma_buf_io_map,
>  						  release_work);
>  	struct dma_buf_io_ctx *ctx = map->ctx;
>  	struct dma_buf *dmabuf = ctx->dmabuf;
>  
>  	dma_resv_lock(dmabuf->resv, NULL);
>  	ctx->dev_ops->unmap(ctx, map);
>  	dma_resv_unlock(dmabuf->resv);
>  
>  	percpu_ref_exit(&map->refs);
>  	kfree(map);

[Severity: Medium]
Since drivers allocate this map object (often embedding it in a larger
structure), does this unconditional kfree(map) restrict where drivers can
embed the structure?

If a driver needs to use vmalloc or embed the map at a non-zero offset,
this will cause a panic. Could a .free callback be provided for safe
lifecycle management?

>  
>  	atomic_dec(&ctx->all_maps);
>  	wake_up(&ctx->maps_wq);

[Severity: High]
Can this sequence lead to a use-after-free on ctx?

If atomic_dec(&ctx->all_maps) drops the count to 0, a concurrent thread
blocked in dma_buf_io_wait_maps() can wake up, return, and free the ctx via
dma_buf_io_ctx_release():

Thread 1 (waiter):
dma_buf_io_ctx_release()
  dma_buf_io_wait_maps()
    wait_event() unblocks since atomic_read(&ctx->all_maps) == 0.
  dma_buf_io_put_ctx() is called and frees ctx.

Thread 2 (worker here):
Proceeds to call wake_up(&ctx->maps_wq) on the now-freed ctx memory.

>  }
[ ... ]
> +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_release, 0,
> +			      GFP_KERNEL);

[Severity: High]
Does this GFP_KERNEL allocation violate DMA-buf locking rules?

dma_buf_io_create_map() calls ctx->dev_ops->map(ctx) while holding the
dmabuf->resv ww_mutex. If the driver then calls dma_buf_io_init_map() under
this lock, the GFP_KERNEL allocation could deadlock if memory reclaim
attempts to acquire dma_resv locks.

> +	if (ret)
> +		return ret;
> +	map->min_seg_shift = seg_shift;
> +	map->ctx = ctx;
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(dma_buf_io_init_map, "DMA_BUF");
> +
> +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;
> +
> +	guard(mutex)(&ctx->map_create_mutex);
> +
> +	scoped_guard(mutex, &ctx->map_mutex) {
> +		if (ctx->maps_killed)
> +			return ERR_PTR(-ENOENT);
> +		/* recheck under the lock in case it has already been re-created */
> +		map = __dma_buf_io_get_map(ctx);
> +		if (map)
> +			return map;
> +	}
> +
> +	dma_buf_io_wait_active_maps(ctx);
> +
> +	ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL,
> +				    true, MAX_SCHEDULE_TIMEOUT);
> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -EAGAIN;
> +		dma_resv_unlock(dmabuf->resv);
> +		return ERR_PTR(ret);
> +	}
> +
> +	map = ctx->dev_ops->map(ctx);
> +	dma_resv_unlock(dmabuf->resv);
> +
> +	if (IS_ERR(map))
> +		return map;
> +	if (WARN_ON_ONCE(!map->min_seg_shift))
> +		return ERR_PTR(-EFAULT);

[Severity: Medium]
If this validation fails and we return -EFAULT, do we leak the map structure
and the underlying device DMA mappings?

It looks like we are bypassing ctx->dev_ops->unmap() and the corresponding
map structure freeing here.

[Severity: Critical]
Does this sequence create a TOCTOU race with dma_buf_io_invalidate_mappings()?

The map is created under dmabuf->resv lock, but we unlock it before updating
ctx->map and ctx->active_maps. 

If an exporter triggers an invalidation:

1. dma_buf_io_create_map() calls ctx->dev_ops->map(ctx).
2. It then calls dma_resv_unlock(dmabuf->resv).
3. The exporter acquires the dmabuf->resv lock and calls
   dma_buf_io_invalidate_mappings().
4. dma_buf_io_kill_maps() and dma_buf_io_wait_active_maps() observe
   no active maps and finish the invalidation.
5. dma_buf_io_create_map() resumes, increments active_maps, and publishes
   the now-stale map to ctx->map.

This allows the stale physical memory mapping to escape to the caller.

> +
> +	atomic_inc(&ctx->active_maps);
> +	atomic_inc(&ctx->all_maps);
> +	/* get a reference for the caller */
> +	percpu_ref_get(&map->refs);
> +
> +	scoped_guard(mutex, &ctx->map_mutex)
> +		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 = kmalloc_obj(*ctx);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	memset(ctx, 0, sizeof(*ctx));
> +	ctx->dir = dir;
> +	ctx->dmabuf = dmabuf;
> +	get_dma_buf(dmabuf);
> +	mutex_init(&ctx->map_mutex);
> +	mutex_init(&ctx->map_create_mutex);
> +	atomic_set(&ctx->active_maps, 0);
> +	atomic_set(&ctx->all_maps, 0);
> +	init_waitqueue_head(&ctx->maps_wq);
> +
> +	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]
If this check fails, does it leak the allocated ctx and the dmabuf reference?

The previous error path handles cleanup via kfree(ctx) and dma_buf_put(dmabuf),
but this one bails out without cleaning up.

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

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