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


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