Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

(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/