Re: [PATCH v3 07/11] dma-buf: heaps: Add debugfs support

[sashiko-bot@kernel.org]

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 <treding@nvidia.com>

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/20260701-tegra-vpr-v3-0-d80f7b871bb4@nvidia.com?part=7