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