Re: [PATCH v4 1/4] dmaengine: add dma_device_get() helper

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Double list_del_rcu() on device->global_node causing kernel panic.
- [High] Use-after-free in dma_chan_put() when dereferencing chan->device->owner.
- [High] Unbalanced dma_device_put() in dma_chan_put() causes kref underflow.
--

--- Patch [1]: [PATCH v4 1/4] dmaengine: add dma_device_get() helper ---
commit bb05480fd3a83327685100c3fb961c2f1ceadfda
Author: Shivank Garg <shivankg@amd.com>

dmaengine: add dma_device_get() helper

This commit adds a dma_device_get() helper function to pair with the existing
dma_device_put() for symmetry. It wraps the open-coded kref_get_unless_zero()
and adds a lockdep assertion to ensure dma_list_mutex is held.

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 6ffd8bd82154a..77638dc16e71d 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -433,6 +433,12 @@ static void dma_device_release(struct kref *ref)
>  		device->device_release(device);
>  }

[Severity: Critical]
This is a pre-existing issue, but can this lead to a double free causing a
kernel panic?

If a module is concurrently unloaded while a client queries for DMA channels,
try_module_get() can fail. When it fails, callers like dmaengine_get() remove
the device from the list:

drivers/dma/dmaengine.c:dmaengine_get() {
    ...
    if (err == -ENODEV) {
        /* module removed before we could use it */
        list_del_rcu(&device->global_node);
        break;
    }
    ...
}

The concurrent module unregistration will subsequently call
dma_async_device_unregister(), which drops the device reference and eventually
invokes dma_device_release(). 

However, dma_device_release() unconditionally calls
list_del_rcu(&device->global_node) again on the already unlinked node. Does
this need a check to prevent dereferencing the poisoned list pointers?

>  
> +static bool dma_device_get(struct dma_device *device)
> +{
> +	lockdep_assert_held(&dma_list_mutex);
> +	return kref_get_unless_zero(&device->ref);
> +}
> +
>  static void dma_device_put(struct dma_device *device)
>  {
>  	lockdep_assert_held(&dma_list_mutex);
> @@ -460,8 +466,7 @@ static int dma_chan_get(struct dma_chan *chan)
>  	if (!try_module_get(owner))
>  		return -ENODEV;
>  
> -	ret = kref_get_unless_zero(&chan->device->ref);
> -	if (!ret) {
> +	if (!dma_device_get(chan->device)) {

[Severity: High]
This isn't a bug introduced by this patch, but does this create a kref
underflow?

In dma_chan_get(), if a channel already has clients, it skips acquiring the
device reference but still increments the client count:

drivers/dma/dmaengine.c:dma_chan_get() {
    ...
    /* The channel is already in use, update client count */
    if (chan->client_count) {
        __module_get(owner);
        chan->client_count++;
        return 0;
    }
    ...
}

However, dma_chan_put() appears to call dma_device_put() unconditionally for
every decrement of client_count. Will a shared channel drop the device
reference multiple times while only acquiring it once?


[Severity: High]
This is also a pre-existing issue, but can this sequence result in a
use-after-free?

In dma_chan_put(), the device reference is dropped:

drivers/dma/dmaengine.c:dma_chan_put() {
    ...
    dma_device_put(chan->device);
    module_put(dma_chan_to_owner(chan));
    ...
}

If this is the final reference, the device struct could be freed by
dma_device_put(). The very next line expands dma_chan_to_owner(chan) into
chan->device->owner, dereferencing the potentially freed chan and chan->device
pointers.

Should the owner pointer be cached locally before dropping the device
reference?

>  		ret = -ENODEV;
>  		goto module_put_out;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-dmaengine-kref-fix-v4-0-c6ef991462a0@amd.com?part=1