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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is unprotected list manipulation and channel count increments when dynamically registering DMA channels.
- [High] This problem wasn't introduced by this patch, but the exported function `dma_async_device_channel_register` calls `dma_channel_rebalance` without holding the required `dma_list_mutex`.
--

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

dmaengine: Allow drivers to assign static channel IDs

The dmaengine core assigns channel IDs in registration order. If a driver
skips a hardware channel, chan_id can differ from the hardware numbering
and a client cannot reliably correlate a requested channel with hardware
resources.

> 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
> @@ -1078,6 +1078,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>  					       struct dma_chan *chan,
>  					       const char *name)
>  {
> +	unsigned int id;
>  	int rc;
>  
>  	chan->local = alloc_percpu(typeof(*chan->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 problem wasn't introduced by this patch, but there is unprotected list
manipulation and channel count increments when dynamically registering DMA
channels.

When dynamically registering DMA channels, the idxd driver calls
list_add_tail(&chan->device_node, &dma->channels) in
drivers/dma/idxd/dma.c:idxd_register_dma_channel() and then
dma_async_device_channel_register() calls
drivers/dma/dmaengine.c:__dma_async_device_channel_register() which does:

	chan->client_count = 0;
	device->chancnt++;

	return 0;

Since idxd probes workqueues (WQs) as separate devices, these can be probed
concurrently. Without proper locking protecting the dma_device's channel list
and count, concurrent registrations can result in list corruption and race
conditions, potentially leading to kernel panics or data corruption.

Could we add proper locking to protect the channel list and count during
dynamic channel registration?

[Severity: High]
This problem wasn't introduced by this patch, but the exported function
dma_async_device_channel_register() calls dma_channel_rebalance() without
holding the required dma_list_mutex.

In drivers/dma/dmaengine.c:dma_async_device_channel_register():

int dma_async_device_channel_register(struct dma_device *device,
				      struct dma_chan *chan)
{
	int rc;

	rc = __dma_async_device_channel_register(device, chan, NULL);
	if (rc < 0)
		return rc;

	dma_channel_rebalance();
	return 0;
}

The dma_channel_rebalance() function iterates over the global dma_device_list
using list_for_each_entry(). Because dma_list_mutex is a static unexported
mutex in drivers/dma/dmaengine.c, callers of
dma_async_device_channel_register() cannot hold it.

This leads to an unprotected concurrent traversal of the global DMA device list,
which can race with other drivers registering or unregistering DMA devices.
Could this result in use-after-free or list corruption crashes?

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