lkml.org 
[lkml]   [2007]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [PATCH] DMA: Fix broken device refcounting
    Date
    From
    >From: Williams, Dan J 
    >On Sat, 2007-10-27 at 06:49 -0700, Haavard Skinnemoen wrote:
    >> On Fri, 26 Oct 2007 09:36:17 -0700
    >> Dan Williams <dan.j.williams@intel.com> wrote:
    >>
    >> > @@ -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);
    >>
    >> While I can't see any problems with the rest of the patch, I
    >think this
    >> part is wrong for the same reasons removing the kref_put() from the
    >> class device cleanup function is. I don't see any constraint that
    >> guarantees that dma_chan_cleanup() will always be called before
    >> dma_dev_release(), which means that "chan" may have been freed before
    >> this function gets a chance to run. Please correct me if I'm wrong.
    >
    >Absolutely right, the driver, not dmaengine, frees the memory so there
    >must be a per channel reference on the device to hold off the driver's
    >remove routine.
    >>
    >> Håvard
    >
    >So how about this...
    >
    >---snip---
    >dmaengine: Fix broken device refcounting
    >
    >From: Haavard Skinnemoen <hskinnemoen@atmel.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@atmel.com>
    >[dan.j.williams@intel.com: kill unnecessary client refcounting]
    >Signed-off-by: Dan Williams <dan.j.williams@intel.com>
    >---
    >
    > drivers/dma/dmaengine.c | 17 ++++++-----------
    > 1 files changed, 6 insertions(+), 11 deletions(-)
    >
    >diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
    >index 84257f7..ec7e871 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;
    > }
    > }
    >@@ -276,11 +275,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 +316,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);
    >@@ -401,6 +394,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;
    >
    >

    I tested this in my ioatdma setup and no longer get the panic. I'm good with this if you two are happy with it.

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

    sln
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-10-29 17:45    [W:0.046 / U:0.156 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site