Re: [PATCH v5 4/4] dmaengine: wait for RCU readers before releasing dma_device

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

Pre-existing issues:
- [High] Double `list_del_rcu` on a device's global_node leading to list corruption (`LIST_POISON2` splats) or kernel panic when a provider module is unloaded.
- [High] The exported functions `dma_async_device_channel_register` and `dma_async_device_channel_unregister` call `dma_channel_rebalance()` without holding the required `dma_list_mutex`.
- [High] Lockless TOCTOU race in `dma_issue_pending_all` between `chan->client_count` check and `device_issue_pending` execution.
--

--- Patch [4]: [PATCH v5 4/4] dmaengine: wait for RCU readers before releasing dma_device ---
commit 2e3a690f3e98bd55a6714eab025b6c1af7e3d406
Author: Shivank Garg <[email protected]>

dmaengine: wait for RCU readers before releasing dma_device
    
This patch introduces a synchronize_rcu() call before releasing the
dma_device in dma_device_release(). It ensures concurrent RCU readers in
dma_issue_pending_all() have finished traversing the list before the device
is freed.

Link: https://sashiko.dev/#/patchset/[email protected]

> dma_issue_pending_all() walks the dma_device_list with
> list_for_each_entry_rcu() under rcu_read_lock().

[Severity: High]
This is a pre-existing issue, but can this lead to a lockless TOCTOU race
between the client_count check and the execution of device_issue_pending()?

In dma_issue_pending_all(), the code locklessly checks if chan->client_count
is non-zero before calling device->device_issue_pending(chan). Concurrently,
dma_release_channel() or dmaengine_put() could acquire dma_list_mutex,
decrement client_count to zero, and tear down the descriptor rings. 

If dma_issue_pending_all() stalls between the check and execution, could it
invoke device_issue_pending() on a channel that has just been released and
disabled?

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 9214b9272a552..f9bc3e69fe58b 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -428,6 +428,7 @@ static void dma_device_release(struct kref *ref)
>  
>  	list_del_rcu(&device->global_node);

[Severity: High]
This isn't a bug introduced by this patch, but could this unconditional
removal result in a double list_del_rcu() and list corruption during module
unloading?

If try_module_get() fails in dma_chan_get() during a concurrent module
unload, dmaengine_get() or find_candidate() will manually execute
list_del_rcu(&device->global_node). 

Later, when the unloading module calls dma_async_device_unregister() and
drops the final reference, dma_device_release() is invoked. Will this cause
a second list_del_rcu() on the same node and a kernel panic from
dereferencing LIST_POISON2?

>  	dma_channel_rebalance();

[Severity: High]
This is a pre-existing issue, but is it safe for exported functions to call
dma_channel_rebalance() without holding dma_list_mutex?

When drivers dynamically register or unregister channels using
dma_async_device_channel_register() or dma_async_device_channel_unregister(),
they call dma_channel_rebalance() completely unlocked. 

Since dma_channel_rebalance() mutates the global channel_table and iterates
over dma_device_list without RCU protection, could concurrent structural
modifications to the list lead to data races or list corruption?

> +	synchronize_rcu();
>  
>  	if (device->device_release)
>  		device->device_release(device);

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