Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] dma-buf: memory and reference leak in dma_buf_io_ctx_create missing dev_ops - [High] dma-buf: map leaked on invalid min_seg_shift in __dma_buf_io_create_map - [High] dma-buf: deadlock risk by blocking on I/O inside dma_resv lock - [Medium] dma-buf: resource leak in dma_buf_io_ctx_release() on map check failure --
commit 420452d61f9f92ad48f07cea0dcd04c4e4a65bac Author: Pavel Begunkov <[email protected]> Subject: dma-buf: introduce initial file I/O infrastructure This commit introduces the initial file I/O infrastructure for natively using dma-buf in the read-write path. It adds building blocks like dma_buf_io_ctx and dma_buf_io_map to serve as an interface between upper layer subsystems and the importer driver. Operations like mapping and unmapping are delegated to the importer driver via dma_buf_io_ops. > 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 block indefinitely on hardware I/O completion while holding the dmabuf->resv lock? dma_buf_io_kill_map() is called from the invalidate_mappings callback. If the exporter calls invalidate_mappings while holding the dma_resv lock during memory reclaim, waiting synchronously for all in-flight I/O requests to drop their references could cause an ABBA deadlock. Memory reclaim might try to acquire the same dma_resv lock. Additionally, dynamic importer rules dictate that invalidate_mappings must not block indefinitely, but should instead detach or set fences so the exporter can wait on them. > + 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: High] Does this error path leak the map structure? When map->min_seg_shift is zero, the function returns ERR_PTR directly. It looks like this skips invoking dma_buf_io_map_release(map) or ctx->dev_ops->unmap(ctx, map), which might permanently leak the map structure, its initialized percpu_ref, and any associated hardware DMA mappings. > + > + ret = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, > + true, MAX_SCHEDULE_TIMEOUT); [Severity: High] Can this wait cause a deadlock by blocking other lockers like shrinkers? Since __dma_buf_io_create_map() holds the dma_resv lock, synchronously waiting on dma_resv_wait_timeout() with MAX_SCHEDULE_TIMEOUT might block any other dma_resv locker while waiting for hardware fences to signal. > + if (ret <= 0) { > + if (!ret) > + ret = -EAGAIN; > + dma_buf_io_map_release(map); > + return ERR_PTR(ret); > + } > + > + /* get a reference for the caller */ > + percpu_ref_get(&map->refs); > + 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 = 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: High] Does returning directly here leak the context and the dma_buf reference? After allocating the context and getting a reference to dmabuf, if any of the required device operations are missing, the function returns -EINVAL directly. This appears to leave the allocated ctx unfreed and the dmabuf reference unreleased, without calling any cleanup like kfree() or dma_buf_put(), nor any driver-specific release. > + > + *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] Does returning early from the destructor leak the context and pin the dma_buf? If the ctx->map is still populated during release, returning early here skips the final ctx->dev_ops->release(ctx), dma_buf_put(ctx->dmabuf), and kfree(ctx). Teardown functions shouldn't silently leak resources when they encounter an invalid state. > + > + ctx->dev_ops->release(ctx); > + dma_buf_put(ctx->dmabuf); > + kfree(ctx); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
