From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757671AbcHCMBB (ORCPT ); Wed, 3 Aug 2016 08:01:01 -0400 Received: from www381.your-server.de ([78.46.137.84]:45681 "EHLO www381.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754314AbcHCMA5 (ORCPT ); Wed, 3 Aug 2016 08:00:57 -0400 Subject: Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback To: Fabien Lahoudere References: <1470221967-19350-1-git-send-email-fabien.lahoudere@collabora.co.uk> <8d94a85a-dfe6-469b-321c-bbbf2acdaa5b@metafoo.de> Cc: Hannu Koivisto , Vinod Koul , Dan Williams , "open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM" , open list From: Lars-Peter Clausen Message-ID: <1452f3a7-fb4e-7a38-4cde-e7cce781102b@metafoo.de> Date: Wed, 3 Aug 2016 13:35:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0 MIME-Version: 1.0 In-Reply-To: <8d94a85a-dfe6-469b-321c-bbbf2acdaa5b@metafoo.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Authenticated-Sender: lars@metafoo.de Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/03/2016 01:19 PM, Lars-Peter Clausen wrote: > On 08/03/2016 12:59 PM, Fabien Lahoudere wrote: >> From: Hannu Koivisto >> >> 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 > > 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.