Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Low] debugfs_create_dir failure handling causes namespace pollution. - [Low] Incomplete resource teardown in newly added dma_heap_exit() function. Pre-existing issues: - [High] Use-After-Free due to character device exposure before uniqueness validation completes. -- commit be3535d09b01d53c055d9d5fb7c60f03232a5ac7 Author: Thierry Reding <[email protected]> dma-buf: heaps: Add debugfs support Add a callback to struct dma_heap_ops that heap providers can implement to show information about the state of the heap in debugfs. A top-level directory named "dma_heap" is created in debugfs and individual files will be named after the heaps. > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index a76bf3f8b071a..1ceb6ee8c05a2 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -224,6 +225,46 @@ const char *dma_heap_get_name(struct dma_heap *heap) [ ... ] > +static struct dentry *dma_heap_debugfs_dir; > + > +static void dma_heap_init_debugfs(void) > +{ > + struct dentry *dir; > + > + dir = debugfs_create_dir("dma_heap", NULL); > + if (IS_ERR(dir)) > + return; > + > + dma_heap_debugfs_dir = dir; > +} [Severity: Low] If debugfs_create_dir() returns an error, this early return leaves dma_heap_debugfs_dir as NULL. Later, when dma_heap_add() calls debugfs_create_file() with this NULL parent, won't it create the heap files directly in the debugfs root directory instead of failing safely? [ ... ] > @@ -298,6 +339,13 @@ struct dma_heap *dma_heap_add(const struct > dma_heap_export_info *exp_info) > > /* Add heap to the list */ > list_add(&heap->list, &heap_list); > + > +#ifdef CONFIG_DEBUG_FS > + if (heap->ops && heap->ops->show) > + debugfs_create_file(heap->name, 0444, dma_heap_debugfs_dir, > + heap, &dma_heap_debug_fops); > +#endif > + > mutex_unlock(&heap_list_lock); > > return heap; [Severity: High] This is a pre-existing issue, but doesn't the current design of dma_heap_add() allow a use-after-free? The character device is made live and exposed to userspace early: cdev_init(&heap->heap_cdev, &dma_heap_fops); ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1); But then, if the uniqueness check fails: list_for_each_entry(h, &heap_list, list) { if (!strcmp(h->name, exp_info->name)) { ... goto err3; } The error path frees the heap without revoking the live character device: err0: kfree(heap); If userspace opens the device concurrently before the uniqueness check fails, could it access the freed heap object? [ ... ] > @@ -334,6 +382,14 @@ static int dma_heap_init(void) > } > dma_heap_class->devnode = dma_heap_devnode; > > + dma_heap_init_debugfs(); > + > return 0; > } > subsys_initcall(dma_heap_init); > + > +static void __exit dma_heap_exit(void) > +{ > + dma_heap_exit_debugfs(); > +} > +__exitcall(dma_heap_exit); [Severity: Low] Does this exit function omit teardown steps? It cleans up debugfs but seems to be missing calls to unregister_chrdev_region() and class_destroy() that were set up in dma_heap_init(). If this subsystem is ever built as a loadable module, won't this leak those resources on unload? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
