* [PATCH 0/2] reposition free_irq to avoid access to invalid data
@ 2013-01-07 10:00 Julia Lawall
2013-01-07 10:00 ` [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: " Julia Lawall
2013-01-07 10:00 ` [PATCH 2/2] drivers/crypto/bfin_crc.c: " Julia Lawall
0 siblings, 2 replies; 9+ messages in thread
From: Julia Lawall @ 2013-01-07 10:00 UTC (permalink / raw)
To: linux-kernel; +Cc: kernel-janitors
The data referenced by an interrupt handler should not be freed before the
interrupt is ended.
The semantic match that finds this problem is as follows
(http://coccinelle.lip6.fr/). The basic idea behind this semantic match is
to find cases where the order of the call to free_irq is different than its
order in some error-handling code. This semantic match, however, has a
high rate of false positives, because most of the time the order doesn't
seem to matter.
// <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>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data 2013-01-07 10:00 [PATCH 0/2] reposition free_irq to avoid access to invalid data Julia Lawall @ 2013-01-07 10:00 ` Julia Lawall 2013-01-07 11:09 ` Guennadi Liakhovetski 2013-01-07 10:00 ` [PATCH 2/2] drivers/crypto/bfin_crc.c: " Julia Lawall 1 sibling, 1 reply; 9+ messages in thread From: Julia Lawall @ 2013-01-07 10:00 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: kernel-janitors, Mauro Carvalho Chehab, linux-media, linux-kernel 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. 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); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data 2013-01-07 10:00 ` [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: " Julia Lawall @ 2013-01-07 11:09 ` Guennadi Liakhovetski 2013-01-07 11:18 ` Julia Lawall 2013-01-07 20:08 ` Robert Jarzmik 0 siblings, 2 replies; 9+ messages in thread From: Guennadi Liakhovetski @ 2013-01-07 11:09 UTC (permalink / raw) To: Julia Lawall Cc: kernel-janitors, Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel, Robert Jarzmik (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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data 2013-01-07 11:09 ` Guennadi Liakhovetski @ 2013-01-07 11:18 ` Julia Lawall 2013-01-07 11:33 ` Guennadi Liakhovetski 2013-01-07 20:08 ` Robert Jarzmik 1 sibling, 1 reply; 9+ messages in thread From: Julia Lawall @ 2013-01-07 11:18 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Julia Lawall, kernel-janitors, Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel, Robert Jarzmik 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data 2013-01-07 11:18 ` Julia Lawall @ 2013-01-07 11:33 ` Guennadi Liakhovetski 0 siblings, 0 replies; 9+ messages in thread From: Guennadi Liakhovetski @ 2013-01-07 11:33 UTC (permalink / raw) To: Julia Lawall Cc: kernel-janitors, Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel, Robert Jarzmik On Mon, 7 Jan 2013, Julia Lawall wrote: > 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. Exactly. > > 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. I think it should be safe. In any case we cannot rely on the fact, that free_irq() in the current code happens "soon" after pxa_free_dma(), so, putting it even later will either emphasise our certainty, that we're safe there, or help up catch the bug, since statistically the window will become larger;-) Of course, all of clk_get(), request_mem_region() + ioremap(), kzalloc(), request_irq() would have to be replaced. Thanks Guennadi > > 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 > > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data 2013-01-07 11:09 ` Guennadi Liakhovetski 2013-01-07 11:18 ` Julia Lawall @ 2013-01-07 20:08 ` Robert Jarzmik 2013-01-07 21:09 ` Julia Lawall 1 sibling, 1 reply; 9+ messages in thread From: Robert Jarzmik @ 2013-01-07 20:08 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Julia Lawall, kernel-janitors, Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > (adding Robert to CC) > 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? Agreed that pxa_free_dma() doesn't free anything, that one is easy :) And agreed too for the second part, with a slighly different explanation : - pxa_camera_remove_device() has been called as you said - inside this function, check comment "/* disable capture, disable interrupts */" => this ensures no interrupt can be generated anymore So after pxa_camera_remove_device() has been called, no interrupts can be generated. Yet as you said, it leaves the "almost impossible" scenario : - a user begins a capture - the user closes the capture device and unloads pxa-camera.ko: soc_camera_close() pxa_camera_remove_device() the IRQ line is asserted but doesn't trigger yet the interrupt handler (yes I know, improbable) meanwhile, IRQs are disabled, DMA channels are stopped switch_to(rmmod) => yes I know, impossible, the interrupt handler must be run before, but let's continue for love of discussion ... rmmod pxa-camera pxa_camera_remove() pxa_free_dma() * 3 ----> here the IRQ handler kicks in !!! => pxa_camera_irq() pxa_dma_start_channels() ----> it hurts ! My call is that this is impossible because the switch_to() should run the IRQ handler before pxa_camera_remove() is called. So all this to say that I think we're safe, unless a heavy ion or a cosmic ray strikes the PXA :) Cheers. -- Robert ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data 2013-01-07 20:08 ` Robert Jarzmik @ 2013-01-07 21:09 ` Julia Lawall 0 siblings, 0 replies; 9+ messages in thread From: Julia Lawall @ 2013-01-07 21:09 UTC (permalink / raw) To: Robert Jarzmik Cc: Guennadi Liakhovetski, kernel-janitors, Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel On Mon, 7 Jan 2013, Robert Jarzmik wrote: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > (adding Robert to CC) > > 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? > > Agreed that pxa_free_dma() doesn't free anything, that one is easy :) > > And agreed too for the second part, with a slighly different explanation : > - pxa_camera_remove_device() has been called as you said > - inside this function, check comment > "/* disable capture, disable interrupts */" > => this ensures no interrupt can be generated anymore > > So after pxa_camera_remove_device() has been called, no interrupts can be > generated. > > Yet as you said, it leaves the "almost impossible" scenario : > - a user begins a capture > - the user closes the capture device and unloads pxa-camera.ko: > soc_camera_close() > pxa_camera_remove_device() > the IRQ line is asserted but doesn't trigger yet the interrupt handler > (yes I know, improbable) > meanwhile, IRQs are disabled, DMA channels are stopped > switch_to(rmmod) > => yes I know, impossible, the interrupt handler must be run before, but > let's continue for love of discussion ... > rmmod pxa-camera > pxa_camera_remove() > pxa_free_dma() * 3 > ----> here the IRQ handler kicks in !!! > => pxa_camera_irq() > pxa_dma_start_channels() > ----> it hurts ! > > My call is that this is impossible because the switch_to() should run the IRQ > handler before pxa_camera_remove() is called. > > So all this to say that I think we're safe, unless a heavy ion or a cosmic ray > strikes the PXA :) Thanks for the explanation. julia ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] drivers/crypto/bfin_crc.c: reposition free_irq to avoid access to invalid data 2013-01-07 10:00 [PATCH 0/2] reposition free_irq to avoid access to invalid data Julia Lawall 2013-01-07 10:00 ` [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: " Julia Lawall @ 2013-01-07 10:00 ` Julia Lawall 2013-01-20 0:12 ` Herbert Xu 1 sibling, 1 reply; 9+ messages in thread From: Julia Lawall @ 2013-01-07 10:00 UTC (permalink / raw) To: Herbert Xu; +Cc: kernel-janitors, David S. Miller, linux-crypto, linux-kernel 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 bfin_crypto_crc_handler. It may refer to crc->regs, which is released by the iounmap. Furthermore, the second argument to all calls to free_irq is incorrect. It should be the same as the last argument of request_irq, which is crc, rather than crc->dev. The semantic match that finds the first 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 problems in practice; the code just looks suspicious. drivers/crypto/bfin_crc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c index a22f1a9..827913d 100644 --- a/drivers/crypto/bfin_crc.c +++ b/drivers/crypto/bfin_crc.c @@ -694,7 +694,7 @@ out_error_dma: dma_free_coherent(&pdev->dev, PAGE_SIZE, crc->sg_cpu, crc->sg_dma); free_dma(crc->dma_ch); out_error_irq: - free_irq(crc->irq, crc->dev); + free_irq(crc->irq, crc); out_error_unmap: iounmap((void *)crc->regs); out_error_free_mem: @@ -720,10 +720,10 @@ static int bfin_crypto_crc_remove(struct platform_device *pdev) crypto_unregister_ahash(&algs); tasklet_kill(&crc->done_task); - iounmap((void *)crc->regs); free_dma(crc->dma_ch); if (crc->irq > 0) - free_irq(crc->irq, crc->dev); + free_irq(crc->irq, crc); + iounmap((void *)crc->regs); kfree(crc); return 0; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drivers/crypto/bfin_crc.c: reposition free_irq to avoid access to invalid data 2013-01-07 10:00 ` [PATCH 2/2] drivers/crypto/bfin_crc.c: " Julia Lawall @ 2013-01-20 0:12 ` Herbert Xu 0 siblings, 0 replies; 9+ messages in thread From: Herbert Xu @ 2013-01-20 0:12 UTC (permalink / raw) To: Julia Lawall; +Cc: kernel-janitors, David S. Miller, linux-crypto, linux-kernel On Mon, Jan 07, 2013 at 11:00:16AM +0100, 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 bfin_crypto_crc_handler. It may refer > to crc->regs, which is released by the iounmap. > > Furthermore, the second argument to all calls to free_irq is incorrect. It > should be the same as the last argument of request_irq, which is crc, > rather than crc->dev. > > The semantic match that finds the first 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> Patch applied. Thanks. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-01-20 0:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-07 10:00 [PATCH 0/2] reposition free_irq to avoid access to invalid data Julia Lawall 2013-01-07 10:00 ` [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: " Julia Lawall 2013-01-07 11:09 ` Guennadi Liakhovetski 2013-01-07 11:18 ` Julia Lawall 2013-01-07 11:33 ` Guennadi Liakhovetski 2013-01-07 20:08 ` Robert Jarzmik 2013-01-07 21:09 ` Julia Lawall 2013-01-07 10:00 ` [PATCH 2/2] drivers/crypto/bfin_crc.c: " Julia Lawall 2013-01-20 0:12 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox