Re: [PATCH v3 1/3] dmaengine: Fix device kref underflow in dma_chan_put()

On Sun, 2026-08-16 at 16:02 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] dma_chan_put() accesses chan->device->owner after potentially freeing chan and chan->device via dma_device_put().
> - [High] dma_release_channel() accesses `chan` fields after calling `dma_chan_put()`, which may have freed the channel.
> - [High] RCU read-side critical section in `dma_issue_pending_all()` can access a synchronously freed `struct dma_device`.
> --
> 
> --- Patch [1]: [PATCH v3 1/3] dmaengine: Fix device kref underflow in dma_chan_put() ---
> commit 0f010c1f7452238748341b884bda3d3bba2c74ac
> Author: Shivank Garg <[email protected]>
> 
> dmaengine: Fix device kref underflow in dma_chan_put()
> 
> This commit fixes a device reference underflow bug by ensuring the device 
> reference is only dropped on the last put of a channel, matching the behavior 
> of the get path.
> 
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index 6ffd8bd82154a..516d6d9332083 100644
> > --- a/drivers/dma/dmaengine.c
> > +++ b/drivers/dma/dmaengine.c
> > @@ -515,7 +515,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 accessing chan after dma_device_put()
> risk a use-after-free?
> 
> If dma_device_put() drops the last reference to the DMA device, it can
> trigger synchronous release of the device memory, which might also free the
> embedded dma_chan.
> 
> Could dma_chan_to_owner(chan) then dereference freed memory by evaluating
> chan->device->owner?
> 

I'm already fixing this in patch 2 by caching chan->device->owner.

> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does dma_release_channel()
> also access freed memory under the same conditions?
> 
> drivers/dma/dmaengine.c:dma_release_channel() {
> 	...
> 	dma_chan_put(chan);
> 
> 	if (chan->slave) {
> 		sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
> 	...
> }
> 
> If dma_chan_put() frees the channel when the last device reference is dropped,
> is it safe to access chan->slave and chan->dev afterwards to tear down
> the sysfs links?

I addressed this in patch 2, but Sashiko pointed the fix would cause NULL
dereference in stm32_dma3_pm_suspend().
> 
> [Severity: High]
> This is also a pre-existing issue, but does the RCU read side in
> dma_issue_pending_all() race with synchronous device release?
> 
> dma_issue_pending_all() relies on an RCU read lock to traverse the device list:
> 
> drivers/dma/dmaengine.c:dma_issue_pending_all() {
> 	...
> 	rcu_read_lock();
> 	list_for_each_entry_rcu(device, &dma_device_list, global_node) {
> 		if (dma_has_cap(DMA_PRIVATE, device->cap_mask))
> 	...
> }
> 
> However, dma_device_release() unlinks the device and synchronously frees
> it without observing 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);
> 	...
> }
> 
> Could the RCU reader dereference a struct dma_device that was just
> freed by dma_device_release()?
> 
Fixed in Patch 3 by adding synchronize_rcu().

Thanks,
Shivank