lkml.org 
[lkml]   [2013]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data
(adding Robert to CC)

Hi Julia

Thanks for the patch.

On Mon, 7 Jan 2013, Julia Lawall wrote:

> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> The data referenced by an interrupt handler should not be freed before the
> interrupt is ended. The handler is pxa_camera_irq. This handler may call
> pxa_dma_start_channels, which references the channels that are freed on the
> lines before the call to free_irq.

I don't think any data is freed by pxa_free_dma(), it only disables DMA on
a certain channel. Theoretically there could be a different problem:
pxa_free_dma() deactivates DMA, whereas pxa_dma_start_channels() activates
it. But I think we're also protected against that: by the time
pxa_camera_remove() is called, and operation on the interface has been
stopped, client devices have been detached, pxa_camera_remove_device() has
been called, which has also stopped the interface clock. And with clock
stopped no interrupts can be generated. And the case of interrupt having
been generated before clk_disabled() and only delivered to the driver so
much later, that we're already unloading the module, seems really
impossible to me. Robert, you agree?

OTOH, it would be nice to convert also this driver to managed allocations,
which also would include devm_request(_threaded)_irq(), but that would
mean, that free_irq() would be called even later than now, also after
pxa_free_dma().

Speaking about managed allocations, those can be dangerous too: if you
request an IRQ before, say, remapping memory, or if you only use managed
IRQ requesting and ioremap() memory in your driver manually, that would be
wrong. But from a quick grep looks like most (all?) drivers get ir right -
first ioremap(), then request IRQ, but to be certain maybe coccinelle
could run a test for that too;-)

Thanks
Guennadi

> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @fn exists@
> expression list es;
> expression a,b;
> identifier f;
> @@
>
> if (...) {
> ... when any
> free_irq(a,b);
> ... when any
> f(es);
> ... when any
> return ...;
> }
>
> @@
> expression list fn.es;
> expression fn.a,fn.b;
> identifier fn.f;
> @@
>
> *f(es);
> ... when any
> *free_irq(a,b);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> Not compiled. I have not observed the problem in practice; the code just
> looks suspicious.
>
> drivers/media/platform/soc_camera/pxa_camera.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
> index f91f7bf..2a19aba 100644
> --- a/drivers/media/platform/soc_camera/pxa_camera.c
> +++ b/drivers/media/platform/soc_camera/pxa_camera.c
> @@ -1810,10 +1810,10 @@ static int pxa_camera_remove(struct platform_device *pdev)
>
> clk_put(pcdev->clk);
>
> + free_irq(pcdev->irq, pcdev);
> pxa_free_dma(pcdev->dma_chans[0]);
> pxa_free_dma(pcdev->dma_chans[1]);
> pxa_free_dma(pcdev->dma_chans[2]);
> - free_irq(pcdev->irq, pcdev);
>
> soc_camera_host_unregister(soc_host);
>
>

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/


\
 
 \ /
  Last update: 2013-01-07 12:42    [W:0.066 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site