From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757985AbcHCM3e (ORCPT ); Wed, 3 Aug 2016 08:29:34 -0400 Received: from www381.your-server.de ([78.46.137.84]:53946 "EHLO www381.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757566AbcHCM3c (ORCPT ); Wed, 3 Aug 2016 08:29:32 -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> Cc: Hannu Koivisto , Vinod Koul , Dan Williams , "open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM" , open list From: Lars-Peter Clausen Message-ID: <8d94a85a-dfe6-469b-321c-bbbf2acdaa5b@metafoo.de> Date: Wed, 3 Aug 2016 13:19:27 +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: <1470221967-19350-1-git-send-email-fabien.lahoudere@collabora.co.uk> 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 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. - Lars [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b36f09c3c441a6e59eab9315032e7d546571de3f