* [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL
@ 2014-07-19 1:21 Javier Martinez Canillas
2014-07-20 14:18 ` Lars-Peter Clausen
2014-07-22 16:09 ` Vinod Koul
0 siblings, 2 replies; 6+ messages in thread
From: Javier Martinez Canillas @ 2014-07-19 1:21 UTC (permalink / raw)
To: Dan Williams
Cc: Vinod Koul, Lars-Peter Clausen, dmaengine, linux-kernel,
Javier Martinez Canillas
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(-)
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;
--
2.0.0.rc2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL
2014-07-19 1:21 [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL Javier Martinez Canillas
@ 2014-07-20 14:18 ` Lars-Peter Clausen
2014-07-20 17:58 ` Javier Martinez Canillas
2014-07-22 16:09 ` Vinod Koul
1 sibling, 1 reply; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-07-20 14:18 UTC (permalink / raw)
To: Javier Martinez Canillas, Dan Williams
Cc: Vinod Koul, dmaengine, linux-kernel
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL
2014-07-20 14:18 ` Lars-Peter Clausen
@ 2014-07-20 17:58 ` Javier Martinez Canillas
2014-07-21 8:22 ` Lars-Peter Clausen
0 siblings, 1 reply; 6+ messages in thread
From: Javier Martinez Canillas @ 2014-07-20 17:58 UTC (permalink / raw)
To: Lars-Peter Clausen, Dan Williams; +Cc: Vinod Koul, dmaengine, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]
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
[-- Attachment #2: kernel_oops.log --]
[-- Type: text/x-log, Size: 3453 bytes --]
[ 1.973959] Unable to handle kernel NULL pointer dereference at virtual address 00000050
[ 1.981807] pgd = 80004000
[ 1.984496] [00000050] *pgd=00000000
[ 1.988056] Internal error: Oops: 5 [#1] SMP ARM
[ 1.992651] Modules linked in:
[ 1.995689] CPU: 1 PID: 71 Comm: spi2 Not tainted 3.16.0-rc5-next-20140718-00021-ga43ca5e #451
[ 2.004276] task: de5bd400 ti: de63c000 task.ti: de63c000
[ 2.009657] PC is at dma_pl330_rqcb.isra.2+0x14/0x64
[ 2.014599] LR is at pl330_release_channel.part.3+0x34/0xa8
[ 2.020151] pc : [<8027b140>] lr : [<8027b5f8>] psr: 60000193
[ 2.020151] sp : de63de48 ip : de63de68 fp : de63de64
[ 2.031601] r10: 00000000 r9 : 00000000 r8 : 00000000
[ 2.036806] r7 : de4eb270 r6 : 60000113 r5 : 0000000c r4 : de5dd000
[ 2.043312] r3 : de5dd000 r2 : 00000000 r1 : 00020001 r0 : 00000000
[ 2.049819] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
[ 2.057192] Control: 10c5387d Table: 4000406a DAC: 00000015
[ 2.062918] Process spi2 (pid: 71, stack limit = 0xde63c248)
[ 2.068556] Stack: (0xde63de48 to 0xde63e000)
[ 2.072896] de40: de5dd000 de5dd000 0000000c 60000113 de63de7c de63de68
[ 2.081050] de60: 8027b5f8 8027b138 de499300 de49935c de63de9c de63de80 8027b6bc 8027b5d0
[ 2.089204] de80: 8027b66c de499314 de4eb000 00000000 de63deb4 de63dea0 80278c94 8027b678
[ 2.097359] dea0: 00000001 de499314 de63decc de63deb8 80278cf4 80278c44 de4eb2e8 de4eb000
[ 2.105513] dec0: de63dee4 de63ded0 802f0220 80278ca4 802f01f0 de4eb248 de63df1c de63dee8
[ 2.113668] dee0: 802eee8c 802f01fc de4eb224 de4eb248 de63c000 de4eb224 de4eb248 de63c000
[ 2.121822] df00: de63c000 00000000 00000000 00000000 de63df44 de63df20 8003c810 802eee04
[ 2.129977] df20: 00000000 de613b00 00000000 de4eb224 8003c72c 00000000 de63dfac de63df48
[ 2.138131] df40: 8003ca90 8003c738 decdbc00 00000000 de63df6c de4eb224 00000000 00000000
[ 2.146286] df60: dead4ead ffffffff ffffffff de63df6c de63df6c 00000000 00000000 dead4ead
[ 2.154440] df80: ffffffff ffffffff de63df88 de63df88 de613b00 8003c998 00000000 00000000
[ 2.162595] dfa0: 00000000 de63dfb0 8000e3d8 8003c9a4 00000000 00000000 00000000 00000000
[ 2.170749] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 2.178904] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 2.187067] [<8027b140>] (dma_pl330_rqcb.isra.2) from [<8027b5f8>] (pl330_release_channel.part.3+0x34/0xa8)
[ 2.196776] [<8027b5f8>] (pl330_release_channel.part.3) from [<8027b6bc>] (pl330_free_chan_resources+0x50/0xac)
[ 2.206839] [<8027b6bc>] (pl330_free_chan_resources) from [<80278c94>] (dma_chan_put+0x5c/0x60)
[ 2.215513] [<80278c94>] (dma_chan_put) from [<80278cf4>] (dma_release_channel+0x5c/0xa0)
[ 2.223676] [<80278cf4>] (dma_release_channel) from [<802f0220>] (s3c64xx_spi_unprepare_transfer+0x30/0x50)
[ 2.233387] [<802f0220>] (s3c64xx_spi_unprepare_transfer) from [<802eee8c>] (spi_pump_messages+0x94/0x534)
[ 2.243018] [<802eee8c>] (spi_pump_messages) from [<8003c810>] (kthread_worker_fn+0xe4/0x144)
[ 2.251516] [<8003c810>] (kthread_worker_fn) from [<8003ca90>] (kthread+0xf8/0x10c)
[ 2.259153] [<8003ca90>] (kthread) from [<8000e3d8>] (ret_from_fork+0x14/0x20)
[ 2.266350] Code: e92dd878 e24cb004 e92d4000 e8bd4000 (e5904050)
[ 2.272436] ---[ end trace 30023402b522a7ab ]---
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL
2014-07-20 17:58 ` Javier Martinez Canillas
@ 2014-07-21 8:22 ` Lars-Peter Clausen
2014-07-21 8:44 ` Javier Martinez Canillas
0 siblings, 1 reply; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-07-21 8:22 UTC (permalink / raw)
To: Javier Martinez Canillas, Dan Williams
Cc: Vinod Koul, dmaengine, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL
2014-07-21 8:22 ` Lars-Peter Clausen
@ 2014-07-21 8:44 ` Javier Martinez Canillas
0 siblings, 0 replies; 6+ messages in thread
From: Javier Martinez Canillas @ 2014-07-21 8:44 UTC (permalink / raw)
To: Lars-Peter Clausen, Dan Williams; +Cc: Vinod Koul, dmaengine, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL
2014-07-19 1:21 [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL Javier Martinez Canillas
2014-07-20 14:18 ` Lars-Peter Clausen
@ 2014-07-22 16:09 ` Vinod Koul
1 sibling, 0 replies; 6+ messages in thread
From: Vinod Koul @ 2014-07-22 16:09 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Dan Williams, Lars-Peter Clausen, dmaengine, linux-kernel
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
--
~Vinod
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-22 16:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-19 1:21 [PATCH 1/1] dmaengine: pl330: Check if the DMA descriptor is NULL Javier Martinez Canillas
2014-07-20 14:18 ` Lars-Peter Clausen
2014-07-20 17:58 ` Javier Martinez Canillas
2014-07-21 8:22 ` Lars-Peter Clausen
2014-07-21 8:44 ` Javier Martinez Canillas
2014-07-22 16:09 ` Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox