Re: [PATCH] DMA: Fix broken device refcounting

[Shannon Nelson]

On 10/27/07, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> [dan.j.williams@xxxxxxxxx: kill unnecessary client refcounting]
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>
>  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;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Thanks - when I get back in tomorrow morning I'll test this to see
that it gets rid of the panic that I've been getting.

sln
-- 
==============================================
Mr. Shannon Nelson         Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/