Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] dma-buf: deadlock due to synchronous wait for I/O in invalidate_mappings - [Medium] dma-buf: memory leak in dma_buf_io_ctx_create() on invalid dev_ops - [Medium] dma-buf: memory leak of dma_buf_io_map in __dma_buf_io_create_map() - [Medium] dma-buf: memory leak in dma_buf_io_ctx_release() due to early return --
commit 8fdf381fa2ecf3131c663cbe675d98398bdf985e Author: Pavel Begunkov <[email protected]> dma-buf: introduce initial file I/O infrastructure This commit introduces basic building blocks (dma_buf_io_ctx and dma_buf_io_map) to natively use dma-buf in the read-write/IO path. It provides the initial API for drivers and upper layer subsystems to interact with the buffers. > 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 synchronously block on I/O completion while holding the dma_resv lock? dma_buf_io_invalidate_mappings() is intended as an invalidation callback which is invoked while holding the dma_resv lock to evict or move the dma-buf. The dynamic importer contract (include/linux/dma-buf.h) states that importers should set fences for any access they can't disable immediately rather than blocking indefinitely. Since block I/O can be delayed or depend on memory allocation, waiting for it in the memory reclaim/eviction path could potentially cause a deadlock. > + 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: Medium] If this min_seg_shift check fails, does it leak the newly allocated map structure? It appears to return ERR_PTR(-EFAULT) directly without invoking dma_buf_io_map_release(map) or the driver's unmap callback. > + > + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, > + true, MAX_SCHEDULE_TIMEOUT); [ ... ] > +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: Medium] Could this error path leak the context and the dma-buf reference? The early return bypasses kfree(ctx) and dma_buf_put(dmabuf). It also doesn't seem to provide a way for the driver to clean up any internal resources it might have allocated during the init_dma_buf_io_ctx() call. > + > + *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] Could this early return cause a memory leak? If the warning triggers, the function returns immediately and skips the final teardown, meaning ctx->dev_ops->release(ctx), dma_buf_put(ctx->dmabuf), and kfree(ctx) won't be executed. > + > + ctx->dev_ops->release(ctx); > + dma_buf_put(ctx->dmabuf); > + kfree(ctx); > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
