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 for Android: free password hash cracker in your pocket

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

Message-ID: <BAE9DCEF64577A439B3A37F36F9B691C034CC6C7@orsmsx418.amr.corp.intel.com>
Date:	Fri, 26 Oct 2007 09:59:41 -0700
From:	"Nelson, Shannon" <shannon.nelson@...el.com>
To:	"Haavard Skinnemoen" <hskinnemoen@...el.com>
Cc:	"Williams, Dan J" <dan.j.williams@...el.com>,
	<linux-kernel@...r.kernel.org>, <akpm@...ux-foundation.org>
Subject: RE: [PATCH] DMA: Fix broken device refcounting

>From: Haavard Skinnemoen [mailto:hskinnemoen@...el.com] 
>
>When a DMA device is unregistered, its reference count is decremented
>twice for each channel: Once dma_class_dev_release() and once in
>dma_chan_cleanup(). This may result in the DMA device driver's
>remove() function completing before all channels have been cleaned
>up, causing lots of use-after-free fun.
>
>Fix it by incrementing the device's reference count twice for each
>channel during registration.
>
>Signed-off-by: Haavard Skinnemoen <hskinnemoen@...el.com>
>---
>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.
>
> drivers/dma/dmaengine.c |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>index 8248992..302eded 100644
>--- a/drivers/dma/dmaengine.c
>+++ b/drivers/dma/dmaengine.c
>@@ -397,6 +397,8 @@ int dma_async_device_register(struct 
>dma_device *device)
> 			goto err_out;
> 		}
> 
>+		/* One for the channel, one of the class device */
>+		kref_get(&device->refcount);
> 		kref_get(&device->refcount);
> 		kref_init(&chan->refcount);
> 		chan->slow_ref = 0;
>-- 
>1.5.2.5
>

As Dan said, we've been discussing this offline, and hadn't come to an
agreement yet.  My version of the patch is the opposite of yours -
instead of adding a kref_get(), I remove one of the kref_put() calls.
--

When a channel is removed from dmaengine, too many kref_put() calls
are made and the device removal happens too soon, usually causing
a panic.

Signed-off-by: Shannon Nelson <shannon.nelson@...el.com>
---

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

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8248992..144a1b7 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -131,7 +131,6 @@ static void dma_async_device_cleanup(struct kref
*kref);
 static void dma_class_dev_release(struct class_device *cd)
 {
 	struct dma_chan *chan = container_of(cd, struct dma_chan,
class_dev);
-	kref_put(&chan->device->refcount, dma_async_device_cleanup);
 }
 
 static struct class dma_devclass = {
-
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/