Re: [PATCH v7 01/10] dmaengine: Allow drivers to assign static channel IDs

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

Pre-existing issues:
- [High] Memory and ID leak in the error rollback path of dma_async_device_register() when a subset of channels fail to register.
- [High] Directly calling kfree() on a device struct after device_register() fails, leading to kobject resource leaks and potential use-after-free.
- [High] dma_channel_rebalance() is called without holding dma_list_mutex when dynamically registering or unregistering channels.
- [Medium] device->chancnt is incremented without synchronization, leading to data races during dynamic channel registration.
--

--- Patch [1]: [PATCH v7 01/10] dmaengine: Allow drivers to assign static channel IDs ---
commit d88a921ed93cbdf57ab5a802c1f278694ad4200a
Author: Koichiro Den <[email protected]>

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. Let a
driver request an exact channel ID before device registration.

Link: https://lore.kernel.org/r/lm7tadnxsyrypu4mypptlkx5qkytex4qxsijdr5ydud2n3anvf@yakjfjg5ng4u/

> 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 is a pre-existing issue, but does the error rollback path in
dma_async_device_register() leak IDs?

drivers/dma/dmaengine.c:dma_async_device_register() {
...
	err_out:
		/* if we never registered a channel just release the idr */
		if (!device->chancnt) {
			ida_free(&dma_ida, device->dev_id);
			return rc;
		}

		list_for_each_entry(chan, &device->channels, device_node) {
			if (chan->local == NULL)
				continue;
			mutex_lock(&dma_list_mutex);
			chan->dev->chan = NULL;
			mutex_unlock(&dma_list_mutex);
			device_unregister(&chan->dev->device);
			free_percpu(chan->local);
		}
		return rc;
}

If channel registration fails and one or more channels were already successfully
registered (device->chancnt > 0), it skips ida_free(&dma_ida, device->dev_id).

Additionally, the rollback loop unregisters the previously successful channels
but appears to miss calling ida_free(&device->chan_ida, chan->chan_id), which
could permanently leak the channel IDs and their associated memory.


[Severity: High]
This is a pre-existing issue, but is it safe to use kfree() after
device_register() fails?

drivers/dma/dmaengine.c:__dma_async_device_channel_register() {
...
	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);
 err_free_local:
	free_percpu(chan->local);
...
}

If device_register(&chan->dev->device) fails, the error handler directly calls
kfree(chan->dev).

Once device_initialize() has been called (internally by device_register()),
shouldn't put_device() be used to clean up the kobject state and avoid leaking
the device name string?


[Severity: High]
This is a pre-existing issue, but are these list operations safe from data
races?

drivers/dma/dmaengine.c:dma_async_device_channel_register() {
...
	dma_channel_rebalance();
...
}

drivers/dma/dmaengine.c:dma_async_device_channel_unregister() {
...
	dma_channel_rebalance();
...
}

When dynamically registering or unregistering channels, dma_channel_rebalance()
is called without acquiring dma_list_mutex.

Since dma_channel_rebalance() locklessly iterates over the global
dma_device_list, could this cause list corruption if another device is
concurrently registered or unregistered?


[Severity: Medium]
This is a pre-existing issue, but does device->chancnt need synchronization
here?

drivers/dma/dmaengine.c:__dma_async_device_channel_register() {
...
	chan->client_count = 0;
	device->chancnt++;
...
}

The increment device->chancnt++ is performed without holding dma_list_mutex or
any other lock.

If multiple channels are dynamically registered concurrently on the same device,
could updates to this counter be lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1