Re: [PATCH v4 2/4] dmaengine: Fix device kref underflow in dma_chan_put()

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

Pre-existing issues:
- [High] Dereferencing `chan->device->owner` in `dma_chan_put()` after `dma_device_put(chan->device)` may access freed device memory.
- [High] Accessing `chan` members in `dma_release_channel()` after dropping the final reference in `dma_chan_put()`.
- [High] Traversing the RCU list in `dma_issue_pending_all` races with device memory freeing due to a missing RCU grace period.
- [High] New channels can be requested and allocated from a DMA device that has already been unregistered and unbound.
--

--- Patch [2]: [PATCH v4 2/4] dmaengine: Fix device kref underflow in dma_chan_put() ---
commit 7dc19aec12ae9d7422f3459f7a5276b21afb0ad9
Author: Shivank Garg <[email protected]>

dmaengine: Fix device kref underflow in dma_chan_put()

This commit addresses a device reference counting underflow by ensuring
dma_device_put() is only called when chan->client_count reaches zero. It
matches the slow-path reference taken in dma_chan_get().

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 77638dc16e71d..3ae53e11e54a6 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -520,7 +520,9 @@ static void dma_chan_put(struct dma_chan *chan)
>  		chan->route_data = NULL;
>  	}
>  
> -	dma_device_put(chan->device);
> +	/* This channel is not in use anymore, drop the device ref */
> +	if (!chan->client_count)
> +		dma_device_put(chan->device);
>  	module_put(dma_chan_to_owner(chan));

[Severity: High]
This is a pre-existing issue, but does this code risk a use-after-free of the
device memory?

When dma_device_put() drops the final reference, the driver's release callback
can free the device memory. Evaluating dma_chan_to_owner(chan) immediately
afterward will dereference chan->device->owner from the potentially freed
device.

[Severity: High]
This isn't a bug introduced by this patch, but can we access a freed channel
structure in dma_release_channel()?

dma_release_channel() calls dma_chan_put(chan). If this drops the last device
reference, the DMA device and its embedded dma_chan structures can be freed.

Immediately following this, dma_release_channel() accesses the channel to
perform cleanup tasks:

drivers/dma/dmaengine.c:dma_release_channel() {
	...
	dma_chan_put(chan);

	if (chan->slave) {
		sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
		sysfs_remove_link(&chan->slave->kobj, chan->name);
		kfree(chan->name);
	...
}

Does this sequence cause memory corruption if the channel is freed?

[Severity: High]
This is a pre-existing issue, but is there a race condition between
traversing the RCU list and freeing device memory?

dma_issue_pending_all() iterates over dma_device_list under an RCU read lock:

drivers/dma/dmaengine.c:dma_issue_pending_all() {
	rcu_read_lock();
	list_for_each_entry_rcu(device, &dma_device_list, global_node) {
	...
}

Concurrently, dma_device_release() removes a device from the list and
immediately calls its release callback without waiting for an RCU grace
period:

drivers/dma/dmaengine.c:dma_device_release() {
	...
	list_del_rcu(&device->global_node);
	dma_channel_rebalance();

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

If the driver synchronously frees the memory, could the concurrent traversal
in dma_issue_pending_all() dereference a freed pointer?

[Severity: High]
This isn't a bug introduced by this patch, but can new channels be requested
from a DMA device that has already been unregistered?

When a driver is unbound, dma_async_device_unregister() sets DMA_PRIVATE
to hide the device and drops its kref:

drivers/dma/dmaengine.c:dma_async_device_unregister() {
	...
	dma_cap_set(DMA_PRIVATE, device->cap_mask);
	...
}

However, if the device has other active channels, it remains in the list.
When __dma_request_channel() searches for a candidate, it calls
private_candidate():

drivers/dma/dmaengine.c:private_candidate() {
	...
	if (dev->chancnt > 1 && !dma_has_cap(DMA_PRIVATE, dev->cap_mask))
		list_for_each_entry(chan, &dev->channels, device_node) {
	...
}

Because DMA_PRIVATE is set, this multi-channel exclusivity check is bypassed.
Could this allow an idle channel to be allocated and its hardware accessed
even after the driver has completed its cleanup and powered off the hardware?

>  }

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