Re: [PATCH v2 3/3] dmaengine: add union chan_dev for dma_chan::dev for clarity

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Improper cleanup on `device_register()` failure leads to memory leaks and potential Use-After-Free.
- [High] Missing `ida_free()` in the error teardown path of `dma_async_device_register()` leads to permanent leakage of the device ID and successfully registered channel IDs.
- [High] Channel devices are modified after they have been globally registered and exposed to the system.
--

commit bdff887fbc44608c64e51be1b2c615d4ee5aaa43
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: add union chan_dev for dma_chan::dev for clarity

This commit introduces a union chan_dev in the dma_chan structure to
explicitly separate the per-channel device from the DMA engine device. It
updates various DMA drivers to use chan_dev instead of dev to improve
clarity, with no intended functional changes.

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

[ ... ]

> @@ -1122,7 +1122,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>   err_out_ida:
>  	ida_free(&device->chan_ida, chan->chan_id);
>   err_free_dev:
> -	kfree(chan->dev);
> +	kfree(chan->chan_dev);
>   err_free_local:
>  	free_percpu(chan->local);

[Severity: High]
This isn't a bug introduced by this patch, but does this error path properly
surrender the device reference?

If device_register() fails earlier in __dma_async_device_channel_register(),
the driver core API requires calling put_device() to surrender the initialized
reference.

Instead, the error path jumps to err_free_dev and directly calls kfree() on
the structure. Can this leak resources allocated during initialization, such
as the device name string, or risk a use-after-free if other subsystems
obtained a temporary reference?

[ ... ]

> @@ -1290,9 +1290,9 @@ int dma_async_device_register(struct dma_device *device)
>  		if (chan->local == NULL)
>  			continue;
>  		mutex_lock(&dma_list_mutex);
> -		chan->dev->chan = NULL;
> +		chan->chan_dev->chan = NULL;
>  		mutex_unlock(&dma_list_mutex);
> -		device_unregister(&chan->dev->device);
> +		device_unregister(&chan->chan_dev->device);
>  		free_percpu(chan->local);
>  	}
>  	return rc;

[Severity: High]
This is a pre-existing issue, but does the error teardown path in
dma_async_device_register() completely release the IDAs?

If channel registration fails after registering at least one channel, the
error path triggers with device->chancnt > 0. This bypasses the earlier block
that calls ida_free(&dma_ida, device->dev_id).

Furthermore, this loop unregisters previously successful channels but appears
to miss calling ida_free(&device->chan_ida, chan->chan_id). Might this
permanently leak the device ID and successfully registered channel IDs?

> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
> index 64cedef1050ae..e36111e8a76dc 100644
> --- a/drivers/dma/tegra186-gpc-dma.c
> +++ b/drivers/dma/tegra186-gpc-dma.c
> @@ -1537,7 +1537,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	 * the channels available and registered for the DMA device are used.
>  	 */
>  	list_for_each_entry(chan, &tdma->dma_dev.channels, device_node) {
> -		chdev = &chan->dev->device;
> +		chdev = &chan->chan_dev->device;
>  		tdc = to_tegra_dma_chan(chan);
>  
>  		if (use_iommu_map) {

[Severity: High]
This is a pre-existing issue, but is it safe to modify fundamental device
properties after the device has already been published to the system?

In tegra_dma_probe(), dmaenginem_async_device_register() is called prior to
this loop, which globally registers the channel devices via device_register().

Here in the loop, when the devices are already visible to sysfs, udev, and
the driver core, chdev->bus is modified and the IOMMU is configured. Could
this create a race condition where the driver core or userspace might
interact with a partially initialized device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-dma-chan_dev-rename-v2-0-0db305b6c716@nxp.com?part=3