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

[Julia Lawall <julia.lawall@lip6.fr>]

On Mon, 7 Jan 2013, Guennadi Liakhovetski wrote:

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

OK, I seem to have been thrown off by the clearing fo the name field, but
that doesn't seem to be very important.

> 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?

OK, thanks for the explanation.

> 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().

OK, if it is safe to call free_irq much later, then I can propose a patch
for that.

> 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;-)

Sure.  Thanks for the suggestion!

julia

> 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/
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>