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 <[email protected]> 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/[email protected]?part=1
