Re: [PATCH v3 3/3] dmaengine: wait for RCU readers before releasing dma_device

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

Pre-existing issues:
- [High] Double list_del_rcu() causes list corruption when DMA provider module unloads concurrently with a channel request.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition between `dma_issue_pending_all()` and `dma_chan_put()` leads to memory corruption by operating on freed DMA channel resources.
--

--- Patch [3]: [PATCH v3 3/3] dmaengine: wait for RCU readers before releasing dma_device ---
commit 6738eea6e01509c5c24f2cdb086fe3fd0ffab13a
Author: Shivank Garg <[email protected]>

dmaengine: wait for RCU readers before releasing dma_device

dma_issue_pending_all() walks the dma_device_list with
list_for_each_entry_rcu() under rcu_read_lock(). dma_device_release()
unlinks the device with list_del_rcu() and then calls
device->device_release(). This patch adds synchronize_rcu() before
freeing the device so RCU readers are guaranteed to have finished.

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

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index bf491eb10596d..29b128aa0beaa 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -429,6 +429,12 @@ 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 can this lead to a double
list_del_rcu() execution and list corruption if the module is unloaded
concurrently with a channel request?

If a thread calls dmaengine_get() or find_candidate(), it iterates
dma_device_list and calls dma_chan_get(). If another thread concurrently
unloads the module:

1. dma_chan_get() calls try_module_get(), which fails and returns -ENODEV
2. dmaengine_get() evaluates -ENODEV and eagerly deletes the device via
   list_del_rcu(&device->global_node)
3. Later, the unloading thread unregisters the device, which invokes
   dma_device_release()
4. dma_device_release() unconditionally calls
   list_del_rcu(&device->global_node) again on the already removed node

Calling list_del_rcu() twice on the same node poisons the prev pointer,
which can cause a panic in __list_del_entry().

>  	dma_channel_rebalance();
>  
> +	/*
> +	 * Wait for RCU readers (e.g. dma_issue_pending_all()) that may still
> +	 * be traversing dma_device_list before the device is freed.
> +	 */
> +	synchronize_rcu();

[Severity: High]
This is a pre-existing issue, but does dma_issue_pending_all() still have
a TOCTOU race condition with dma_chan_put() that could lead to memory
corruption?

While this synchronize_rcu() protects the dma_device itself during driver
unbind, it doesn't seem to protect against channel release.

If dma_issue_pending_all() evaluates chan->client_count locklessly
under rcu_read_lock():

Thread A:
dma_issue_pending_all()
    if (chan->client_count)
        // preempted here

Thread B:
dma_release_channel()
    dma_chan_put()
        // drops client_count to 0
        chan->device->device_free_chan_resources(chan);

Thread A resumes:
    device->device_issue_pending(chan);

The driver writes to the hardware, forcing it to fetch from the newly
freed DMA ring. Can the hardware be safely started here if the resources
were just freed?

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

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