| Message ID | 1405732907-30964-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b1e51d771fbc |
| Delegated to: | Vinod Koul |
| Headers | show |
On 07/19/2014 03:21 AM, Javier Martinez Canillas wrote: > Commit 6079d38 ("dmaengine: pl330: Remove useless xfer_cb indirection") > removed the __callback() function which created an unnecessary level of > indirection to execute the tranfer callback .xfer_cb > > Unfortunately the commit also changed the semantics slightly since that > function used to check if the request was not NULL before attempting to > execute the callback function. Not checking this could lead to a kernel > NULL pointer dereference error. This should not happen, but I guess it can happen when terminal_all() is called. (It's wrong to try to complete a descriptor from terminal_all() in the first place, but that's a different issue) > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Acked-by: Lars-Peter Clausen <lars@metafoo.de> > --- > drivers/dma/pl330.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index bc5878a..a55d754 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -1441,9 +1441,14 @@ xfer_exit: > > static void dma_pl330_rqcb(struct dma_pl330_desc *desc, enum pl330_op_err err) > { > - struct dma_pl330_chan *pch = desc->pchan; > + struct dma_pl330_chan *pch; > unsigned long flags; > > + if (!desc) > + return; > + > + pch = desc->pchan; > + > /* If desc aborted */ > if (!pch) > return; > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Lars-Peter, On 07/20/2014 04:18 PM, Lars-Peter Clausen wrote: > On 07/19/2014 03:21 AM, Javier Martinez Canillas wrote: >> Commit 6079d38 ("dmaengine: pl330: Remove useless xfer_cb indirection") >> removed the __callback() function which created an unnecessary level of >> indirection to execute the tranfer callback .xfer_cb >> >> Unfortunately the commit also changed the semantics slightly since that >> function used to check if the request was not NULL before attempting to >> execute the callback function. Not checking this could lead to a kernel >> NULL pointer dereference error. > > This should not happen, but I guess it can happen when terminal_all() is I should had mentioned before that this patch is not trying to fix a theoretical issue but a kernel oops when booting linux next-20140718 on a Exynos5420 SoC based Chromebook 2 machine. I'm sending as an attachment the complete kernel crash log but the problem happens when the spi_master .unprepare_transfer_hardware function handler in the spi-s3c64xx driver tries to release a DMA channel: s3c64xx_spi_unprepare_transfer() -> dma_release_channel() -> dma_chan_put() -> chan->device->device_free_chan_resources() -> pl330_free_chan_resources() -> pl330_release_channel() -> dma_pl330_rqcb() > called. (It's wrong to try to complete a descriptor from terminal_all() in > the first place, but that's a different issue) If this should not really happen and this patch is only a workaround since the bug is elsewhere, please give me some hints and I'll try to fix it properly. I'm not familiar with the PL330 DMA controller but just found what was the NULL pointer being dereferenced and looked at your changes to see what was different now. > >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > > Acked-by: Lars-Peter Clausen <lars@metafoo.de> > Thanks a lot and best regards, Javier
On 07/20/2014 07:58 PM, Javier Martinez Canillas wrote: > Hello Lars-Peter, > > On 07/20/2014 04:18 PM, Lars-Peter Clausen wrote: >> On 07/19/2014 03:21 AM, Javier Martinez Canillas wrote: >>> Commit 6079d38 ("dmaengine: pl330: Remove useless xfer_cb indirection") >>> removed the __callback() function which created an unnecessary level of >>> indirection to execute the tranfer callback .xfer_cb >>> >>> Unfortunately the commit also changed the semantics slightly since that >>> function used to check if the request was not NULL before attempting to >>> execute the callback function. Not checking this could lead to a kernel >>> NULL pointer dereference error. >> >> This should not happen, but I guess it can happen when terminal_all() is > > I should had mentioned before that this patch is not trying to fix a theoretical > issue but a kernel oops when booting linux next-20140718 on a Exynos5420 SoC > based Chromebook 2 machine. > > I'm sending as an attachment the complete kernel crash log but the problem > happens when the spi_master .unprepare_transfer_hardware function handler in the > spi-s3c64xx driver tries to release a DMA channel: > > s3c64xx_spi_unprepare_transfer() -> > dma_release_channel() -> > dma_chan_put() -> > chan->device->device_free_chan_resources() -> > pl330_free_chan_resources() -> > pl330_release_channel() -> > dma_pl330_rqcb() > >> called. (It's wrong to try to complete a descriptor from terminal_all() in >> the first place, but that's a different issue) > > If this should not really happen and this patch is only a workaround since the > bug is elsewhere, please give me some hints and I'll try to fix it properly. I'm > not familiar with the PL330 DMA controller but just found what was the NULL > pointer being dereferenced and looked at your changes to see what was different now. I think the patch is fine as a quick workaround since it is simple and the previous commit broke previously working code. The long term fix is to stop calling dma_pl330_rqcb() from pl330_release_channel(). The first thing is you wouldn't expect any transfer to be active when the channel is released. And even if it was by accident we should not call the descriptor callback, but rather but it just back onto the descriptor pool. - Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/21/2014 10:22 AM, Lars-Peter Clausen wrote: > On 07/20/2014 07:58 PM, Javier Martinez Canillas wrote: >> >> If this should not really happen and this patch is only a workaround since the >> bug is elsewhere, please give me some hints and I'll try to fix it properly. I'm >> not familiar with the PL330 DMA controller but just found what was the NULL >> pointer being dereferenced and looked at your changes to see what was different now. > > I think the patch is fine as a quick workaround since it is simple and the > previous commit broke previously working code. > Agreed, it matches what the old code was doing and other code is relying on this behavior so works as a quick fix to avoid the current kernel oops. > The long term fix is to stop calling dma_pl330_rqcb() from > pl330_release_channel(). The first thing is you wouldn't expect any transfer > to be active when the channel is released. And even if it was by accident we > should not call the descriptor callback, but rather but it just back onto > the descriptor pool. Thanks a lot for the clarification. I thought it was something along the lines of not calling the callback from pl330_release_channel() but preferred to not change anything that I could not completely understand its side effects. I'll study the driver more deeply and try to come up with a patch on top of this one that fixes the actual cause rather than the consequence. > > - Lars > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 19, 2014 at 03:21:47AM +0200, Javier Martinez Canillas wrote: > Commit 6079d38 ("dmaengine: pl330: Remove useless xfer_cb indirection") > removed the __callback() function which created an unnecessary level of > indirection to execute the tranfer callback .xfer_cb > > Unfortunately the commit also changed the semantics slightly since that > function used to check if the request was not NULL before attempting to > execute the callback function. Not checking this could lead to a kernel > NULL pointer dereference error. Applied, thanks
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index bc5878a..a55d754 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1441,9 +1441,14 @@ xfer_exit: static void dma_pl330_rqcb(struct dma_pl330_desc *desc, enum pl330_op_err err) { - struct dma_pl330_chan *pch = desc->pchan; + struct dma_pl330_chan *pch; unsigned long flags; + if (!desc) + return; + + pch = desc->pchan; + /* If desc aborted */ if (!pch) return;
Commit 6079d38 ("dmaengine: pl330: Remove useless xfer_cb indirection") removed the __callback() function which created an unnecessary level of indirection to execute the tranfer callback .xfer_cb Unfortunately the commit also changed the semantics slightly since that function used to check if the request was not NULL before attempting to execute the callback function. Not checking this could lead to a kernel NULL pointer dereference error. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/dma/pl330.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)