linux-kernel - Re: [PATCH] DMA: Fix broken device refcounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 26 Oct 2007 09:36:17 -0700
From:	Dan Williams <dan.j.williams@...el.com>
To:	Haavard Skinnemoen <hskinnemoen@...el.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Shannon Nelson <shannon.nelson@...el.com>
Subject: Re: [PATCH] DMA: Fix broken device refcounting


On Fri, 2007-10-26 at 09:12 -0700, Haavard Skinnemoen wrote:
> I'm not sure if this is the correct way to solve it, but it seems to
> work. The remove() function does not hang, which indicates that the
> device's reference count does drop all the way to zero on
> unregistration, which in turn indicates that it did actually drop
> _below_ zero before.

Yeah, Shannon ran into this too...  I'd like to be able clean this up by
reducing the number of time we take the device reference, but the
following patch is still showing problems in Shannon's environment, so I
missed one...

---

dmaengine: fix up dma_device refcounting

From: Dan Williams <dan.j.williams@...el.com>

Currently the code drops too many references on the parent device.  Change
the scheme to:

+ take a reference at registration:
  dma_async_device_register()
+ take a reference for each channel device registered:
  device_register(&chan->dev)
- drop a reference for each channel device unregistered:
  device_unregister(&chan->dev)
- drop a reference at unregistration:
  dma_async_device_unregister()

Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---

 drivers/dma/dmaengine.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 84257f7..d2b600b 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -186,10 +186,9 @@ static void dma_client_chan_alloc(struct dma_client *client)
 				/* we are done once this client rejects
 				 * an available resource
 				 */
-				if (ack == DMA_ACK) {
+				if (ack == DMA_ACK)
 					dma_chan_get(chan);
-					kref_get(&device->refcount);
-				} else if (ack == DMA_NAK)
+				else if (ack == DMA_NAK)
 					return;
 			}
 		}
@@ -221,7 +220,6 @@ void dma_chan_cleanup(struct kref *kref)
 {
 	struct dma_chan *chan = container_of(kref, struct dma_chan, refcount);
 	chan->device->device_free_chan_resources(chan);
-	kref_put(&chan->device->refcount, dma_async_device_cleanup);
 }
 EXPORT_SYMBOL(dma_chan_cleanup);
 
@@ -276,11 +274,8 @@ static void dma_clients_notify_removed(struct dma_chan *chan)
 		/* client was holding resources for this channel so
 		 * free it
 		 */
-		if (ack == DMA_ACK) {
+		if (ack == DMA_ACK)
 			dma_chan_put(chan);
-			kref_put(&chan->device->refcount,
-				dma_async_device_cleanup);
-		}
 	}
 
 	mutex_unlock(&dma_list_mutex);
@@ -320,11 +315,8 @@ void dma_async_client_unregister(struct dma_client *client)
 			ack = client->event_callback(client, chan,
 				DMA_RESOURCE_REMOVED);
 
-			if (ack == DMA_ACK) {
+			if (ack == DMA_ACK)
 				dma_chan_put(chan);
-				kref_put(&chan->device->refcount,
-					dma_async_device_cleanup);
-			}
 		}
 
 	list_del(&client->global_node);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/