Re: [PATCH v6 08/12] dma-buf: heaps: Add debugfs support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete resource cleanup in the newly added `dma_heap_exit()` leaves dynamically allocated heaps, character devices, and sysfs devices active but orphaned.

Pre-existing issues:
- [High] Race condition in `dma_heap_add` error path leads to a critical Use-After-Free of `struct dma_heap` due to premature device exposure.
--

commit e23fa30f3769a236faddd443565a8ccf6a9af125
Author: Thierry Reding <treding@nvidia.com>

dma-buf: heaps: Add debugfs support

This commit adds a callback to struct dma_heap_ops to show heap state in
debugfs. It creates a top-level dma_heap directory where individual heap
files are registered. A new dma_heap_exit() function is also introduced to
clean up the class and debugfs directory on module exit.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 8a6c2b6e0cc60..65c80dac65009 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c

[ ... ]

> @@ -299,6 +334,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 isn't a bug introduced by this patch, but is it possible for a concurrent
userspace process to trigger a use-after-free during the error path of
dma_heap_add()?

Earlier in dma_heap_add(), the character device is exposed to userspace before
the name uniqueness check is performed:

	ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1);
	...
	dev_ret = device_create(dma_heap_class, ...);
	...
	/* check the name is unique */
	list_for_each_entry(h, &heap_list, list) {
		if (!strcmp(h->name, exp_info->name)) {
			...
			goto err3;
		}
	}

If another driver attempts to register a heap with a duplicate name, could a
userspace process open the device before the uniqueness check fails? This would
store the heap pointer in the open file descriptor. When the uniqueness check
then fails, the error path executes kfree(heap), leaving the open file
descriptor pointing to freed memory.

[ ... ]

> @@ -335,6 +377,16 @@ 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();
> +	class_destroy(dma_heap_class);

[Severity: Medium]
Does this successfully tear down the resources if there are dynamically
allocated heaps still registered?

The dma_heap_exit() function destroys the sysfs class by calling
class_destroy(), but it doesn't appear to iterate over heap_list to cleanly
tear down the individual registered heaps. 

Can this leave dangling references and lead to sysfs issues, since
device_destroy() and cdev_del() are never called for any active heaps before
the class is destroyed?

> +	unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
> +}
> +__exitcall(dma_heap_exit);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-tegra-vpr-v6-0-79042cfa8de5@nvidia.com?part=8