Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] dma-buf: map leak and deadlock on invalid min_seg_shift - [Medium] dma-buf: memory leak in dma_buf_io_ctx_create() dev_ops validation --
commit cebbd64805f3eac146806ef3dc1344fdf0a76c98 Author: Pavel Begunkov <[email protected]> dma-buf: introduce initial file I/O infrastructure This patch adds basic building blocks serving as an API between drivers and upper layer subsystems to natively use dma-buf in the IO paths. It introduces the dma_buf_io_ctx and dma_buf_io_map structures to handle mapping, unmapping, and invalidation operations. > 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 [ ... ] > +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)) { > + atomic_inc(&ctx->active_maps); > + atomic_inc(&ctx->all_maps); > + } > + dma_resv_unlock(dmabuf->resv); > + > + if (IS_ERR(map)) > + return map; > + if (WARN_ON_ONCE(!map->min_seg_shift)) > + return ERR_PTR(-EFAULT); [Severity: High] Does this error path leak the map and deadlock the system? If map->min_seg_shift evaluates to 0 (for example, if an odd-length DMA segment causes __ffs() to return 0), the WARN_ON_ONCE check will trigger. Since atomic_inc(&ctx->active_maps) and atomic_inc(&ctx->all_maps) have already executed for a valid map, returning ERR_PTR(-EFAULT) here skips dropping the map's percpu_ref count and leaves the atomic counters permanently elevated. Could subsequent cleanup calls to dma_buf_io_wait_active_maps() or dma_buf_io_wait_maps() then block forever waiting for the counters to reach zero? [ ... ] > +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: Medium] Can this early return leak the context and the dma-buf reference? If the init_dma_buf_io_ctx callback succeeds but fails to populate the required dev_ops, this path returns -EINVAL immediately. It appears to skip the kfree(ctx) and dma_buf_put(dmabuf) teardown operations that are present in the preceding error handling block. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
