Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1455743 > unrolled thread

[PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2016-08-03 13:00 +0200
    Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA  callback Lars-Peter Clausen <lars@metafoo.de> - 2016-08-03 14:10 +0200
    Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA  callback Lars-Peter Clausen <lars@metafoo.de> - 2016-08-03 14:30 +0200

#1455743 — [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback

From: Hannu Koivisto <hannu.koivisto@vincit.fi>

dma_rx_callback() may see NULL dma_chan_rx if DMA interrupt [1] occurs a
moment[2] before imx_uart_dma_exit() sets it to NULL.  imx_uart_dma_exit()
calls dmaengine_terminate_all() and dma_release_channel() but neither of
those prevent the callback being called after they have returned. A similar
problem has been discussed by ALSA developers
(http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067239.html)
and it was pointed out that dmaengine_terminate_all() might be called from
the callback, so we cannot call tasklet_kill() in imx-sdma's code called by
dmaengine_terminate_all().

Hopefully it doesn't make sense to call dma_release_channel() from the
callback, so instead of adding synchronization to imx serial driver, we add
tasklet_kill() call to sdma_free_chan_resources().  While most DMA drivers
don't do that, there is one example that does: pl330.

[1] It schedules sdma_tasklet, which again calls the dma_rx_callback.
[2] I tested this by scheduling the sdma tasklet as far as right before the
imx_stop_tx() call in imx_shutdown() and the problem occurred.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
---
 drivers/dma/imx-sdma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 46c9027..da86792 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1135,6 +1135,8 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 
 	sdma_disable_channel(chan);
 
+	tasklet_kill(&sdmac->tasklet);
+
 	if (sdmac->event_id0)
 		sdma_event_disable(sdmac, sdmac->event_id0);
 	if (sdmac->event_id1)
-- 
2.7.4

[toc] | [next] | [standalone]

#1455767 — Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback

On 08/03/2016 01:19 PM, Lars-Peter Clausen wrote:
> On 08/03/2016 12:59 PM, Fabien Lahoudere wrote:
>> From: Hannu Koivisto <hannu.koivisto@vincit.fi>
>>
>> dma_rx_callback() may see NULL dma_chan_rx if DMA interrupt [1] occurs a
>> moment[2] before imx_uart_dma_exit() sets it to NULL.  imx_uart_dma_exit()
>> calls dmaengine_terminate_all() and dma_release_channel() but neither of
>> those prevent the callback being called after they have returned. A similar
>> problem has been discussed by ALSA developers
>> (http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067239.html)
>> and it was pointed out that dmaengine_terminate_all() might be called from
>> the callback, so we cannot call tasklet_kill() in imx-sdma's code called by
>> dmaengine_terminate_all().
>>
>> Hopefully it doesn't make sense to call dma_release_channel() from the
>> callback, so instead of adding synchronization to imx serial driver, we add
>> tasklet_kill() call to sdma_free_chan_resources().  While most DMA drivers
>> don't do that, there is one example that does: pl330.
>>
>> [1] It schedules sdma_tasklet, which again calls the dma_rx_callback.
>> [2] I tested this by scheduling the sdma tasklet as far as right before the
>> imx_stop_tx() call in imx_shutdown() and the problem occurred.
>>
>> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
> 
> I'd prefer that the driver implements the new synchronization API[1]. This
> is a more generic approach and covers of all cases of this race condition.
> 
> If the synchronize() callback is implemented the core will automatically
> make sure that the channel is synchronized when it is freed.

Looking at the driver it also seems that just calling tasklet_kill() is not
enough. The tasklet_schedule() in the sdma_int_handler() is not synchronized
to anything. So if the interrupt triggers just at the right time it might
re-schedule the tasklet after tasklet_kill() has been called. Especially if
the tasklet_kill() runs on a different CPU.

[toc] | [prev] | [next] | [standalone]

#1455781 — Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback

On 08/03/2016 12:59 PM, Fabien Lahoudere wrote:
> From: Hannu Koivisto <hannu.koivisto@vincit.fi>
> 
> dma_rx_callback() may see NULL dma_chan_rx if DMA interrupt [1] occurs a
> moment[2] before imx_uart_dma_exit() sets it to NULL.  imx_uart_dma_exit()
> calls dmaengine_terminate_all() and dma_release_channel() but neither of
> those prevent the callback being called after they have returned. A similar
> problem has been discussed by ALSA developers
> (http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067239.html)
> and it was pointed out that dmaengine_terminate_all() might be called from
> the callback, so we cannot call tasklet_kill() in imx-sdma's code called by
> dmaengine_terminate_all().
> 
> Hopefully it doesn't make sense to call dma_release_channel() from the
> callback, so instead of adding synchronization to imx serial driver, we add
> tasklet_kill() call to sdma_free_chan_resources().  While most DMA drivers
> don't do that, there is one example that does: pl330.
> 
> [1] It schedules sdma_tasklet, which again calls the dma_rx_callback.
> [2] I tested this by scheduling the sdma tasklet as far as right before the
> imx_stop_tx() call in imx_shutdown() and the problem occurred.
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>

I'd prefer that the driver implements the new synchronization API[1]. This
is a more generic approach and covers of all cases of this race condition.

If the synchronize() callback is implemented the core will automatically
make sure that the channel is synchronized when it is freed.

- Lars

[1]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b36f09c3c441a6e59eab9315032e7d546571de3f

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web