Re: [PATCH v2 1/3] dmaengine: Allow drivers to assign static channel IDs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect error handling for `device_register()` violates the driver model API, leading to a memory leak of device internals and potential Use-After-Free.
--

commit a264234e1c749394dbf84d5c9e286ff9f6d8a114
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: Allow drivers to assign static channel IDs

The dmaengine core assigns channel IDs in registration order. Let a driver
request an exact channel ID before device registration. Reserve static IDs
through the existing IDA so they remain unique, while retaining automatic
IDA allocation as the default.

Link: https://lore.kernel.org/r/20260813063757.3131865-2-den@valinux.co.jp/
Link: https://lore.kernel.org/r/an4TXwT37vWYEzCn@SMW015318/

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 6ffd8bd82154a..cc64a4679e6fa 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c

[ ... ]

> @@ -1089,11 +1090,13 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>  		goto err_free_local;
>  	}
>  
> -	/*
> -	 * When the chan_id is a negative value, we are dynamically adding
> -	 * the channel. Otherwise we are static enumerating.
> -	 */
> -	chan->chan_id = ida_alloc(&device->chan_ida, GFP_KERNEL);
> +	if (chan->chan_id & DMA_CHAN_ID_STATIC) {
> +		id = chan->chan_id & ~DMA_CHAN_ID_STATIC;
> +		chan->chan_id = ida_alloc_range(&device->chan_ida, id, id,
> +						GFP_KERNEL);
> +	} else {
> +		chan->chan_id = ida_alloc(&device->chan_ida, GFP_KERNEL);
> +	}
>  	if (chan->chan_id < 0) {
>  		pr_err("%s: unable to alloc ida for chan: %d\n",
>  		       __func__, chan->chan_id);

[Severity: High]
This isn't a bug introduced by this patch, but does this error handling
leak device internals and risk a use-after-free?

Looking at __dma_async_device_channel_register() further down in this
function, if device_register(&chan->dev->device) fails:

	rc = device_register(&chan->dev->device);
	if (rc)
		goto err_out_ida;
	...
 err_out_ida:
	ida_free(&device->chan_ida, chan->chan_id);
 err_free_dev:
	kfree(chan->dev);

When device_register() returns an error, the initial reference taken by
kobject_init() still needs to be dropped. Doesn't calling kfree(chan->dev)
directly skip the internal dev->p cleanup triggered by put_device(),
permanently leaking dev->p?

Also, if the failure occurs late in device_add() after sysfs files have
been exposed, userspace might have opened them. Could freeing the memory
directly while userspace holds a reference to the kobject result in a
use-after-free upon subsequent access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828163611.2691264-1-den@valinux.co.jp?part=1