Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751580AbcLEKa3 (ORCPT ); Mon, 5 Dec 2016 05:30:29 -0500 Received: from mx2.suse.de ([195.135.220.15]:47092 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750816AbcLEKaY (ORCPT ); Mon, 5 Dec 2016 05:30:24 -0500 Date: Mon, 05 Dec 2016 11:30:17 +0100 Message-ID: From: Takashi Iwai To: Jiada Wang Cc: , , , , Subject: Re: [PATCH 3/3 v3] ALSA: usb-audio: fix race in snd_usb_endpoint_stop In-Reply-To: <58453D33.5000006@mentor.com> References: <20161130075923.15205-1-jiada_wang@mentor.com> <20161130075923.15205-4-jiada_wang@mentor.com> <58453D33.5000006@mentor.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.1 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8391 Lines: 189 On Mon, 05 Dec 2016 11:10:59 +0100, Jiada Wang wrote: > > Hi, Takashi > > On 11/30/2016 01:00 AM, Takashi Iwai wrote: > > On Wed, 30 Nov 2016 08:59:23 +0100, > > Jiada Wang wrote: > >> From: Mark Craske > >> > >> Kernel crash seen once: > >> > >> Unable to handle kernel NULL pointer dereference at virtual address 00000008 > >> pgd = a1d7c000 > >> [00000008] *pgd=31c93831, *pte=00000000, *ppte=00000000 > >> Internal error: Oops: 17 [#1] PREEMPT SMP ARM > >> CPU: 0 PID: 250 Comm: dbus-daemon Not tainted 3.14.51-03479-gf50bdf4 #1 > >> task: a3ae61c0 ti: a08c8000 task.ti: a08c8000 > >> PC is at retire_capture_urb+0x10/0x1f4 [snd_usb_audio] > >> LR is at snd_complete_urb+0x140/0x1f0 [snd_usb_audio] > >> pc : [<7f0eb22c>] lr : [<7f0e57fc>] psr: 200e0193 > >> sp : a08c9c98 ip : a08c9ce8 fp : a08c9ce4 > >> r10: 0000000a r9 : 00000102 r8 : 94cb3000 > >> r7 : 94cb3000 r6 : 94d0f000 r5 : 94d0e8e8 r4 : 94d0e000 > >> r3 : 7f0eb21c r2 : 00000000 r1 : 94cb3000 r0 : 00000000 > >> Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user > >> Control: 10c5387d Table: 31d7c04a DAC: 00000015 > >> Process dbus-daemon (pid: 250, stack limit = 0xa08c8238) > >> Stack: (0xa08c9c98 to 0xa08ca000) > >> ... > >> Backtrace: > >> [<7f0eb21c>] (retire_capture_urb [snd_usb_audio]) from [<7f0e57fc>] (snd_complete_urb+0x140/0x1f0 [snd_usb_audio]) > >> [<7f0e56bc>] (snd_complete_urb [snd_usb_audio]) from [<80371118>] (__usb_hcd_giveback_urb+0x78/0xf4) > >> [<803710a0>] (__usb_hcd_giveback_urb) from [<80371514>] (usb_giveback_urb_bh+0x8c/0xc0) > >> [<80371488>] (usb_giveback_urb_bh) from [<80028e3c>] (tasklet_hi_action+0xc4/0x148) > >> [<80028d78>] (tasklet_hi_action) from [<80028358>] (__do_softirq+0x190/0x380) > >> [<800281c8>] (__do_softirq) from [<80028858>] (irq_exit+0x8c/0xfc) > >> [<800287cc>] (irq_exit) from [<8000ea88>] (handle_IRQ+0x8c/0xc8) > >> [<8000e9fc>] (handle_IRQ) from [<800085e8>] (gic_handle_irq+0xbc/0xf8) > >> [<8000852c>] (gic_handle_irq) from [<80509044>] (__irq_svc+0x44/0x78) > >> [<80508820>] (_raw_spin_unlock_irq) from [<8004b880>] (finish_task_switch+0x5c/0x100) > >> [<8004b824>] (finish_task_switch) from [<805052f0>] (__schedule+0x48c/0x6d8) > >> [<80504e64>] (__schedule) from [<805055d4>] (schedule+0x98/0x9c) > >> [<8050553c>] (schedule) from [<800116c8>] (do_work_pending+0x30/0xd0) > >> [<80011698>] (do_work_pending) from [<8000e160>] (work_pending+0xc/0x20) > >> Code: e1a0c00d e92ddff0 e24cb004 e24dd024 (e5902008) > >> Kernel panic - not syncing: Fatal exception in interrupt > >> > >> There is a race between retire_capture_urb()& stop_endpoints() which is > >> setting ep->data_subs to NULL. The underlying cause is in > >> snd_usb_endpoint_stop(), which sets > >> ep->data_subs = NULL; > >> ep->sync_slave = NULL; > >> ep->retire_data_urb = NULL; > >> ep->prepare_data_urb = NULL; > >> > >> An improvement, suggested by Andreas Pape (ADIT) is to read parameters > >> into local variables. This should solve race between stop and retire > >> where it is legal to store the pointers to local variable as they are > >> not freed in stop path but just set to NULL. > > Well, it's tricky. A saner way would be to clear the stuff really > > after all users are gone. > > > > An untested patch is below. Let me know if it really works. > Thanks for your proposal, I am afraid, we only found the race issue > once during > our automation test, so I can't test for your patch, > but from what I can see, your patch makes sense to me. > > The only difference when apply with your patch is, now in case > stop_endpoints() is called > from TRIGGER_STOP, these stuff won't get cleared, but I think this > isn't an issue, is it? Right. After the trigger-stop, the PCM state goes to SETUP, so it can't be played from that point immediately, thus the race won't happen. FWIW, below is the patch I'm going to queue. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: usb-audio: Fix race at stopping the stream We've got a kernel crash report showing like: Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = a1d7c000 [00000008] *pgd=31c93831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] PREEMPT SMP ARM CPU: 0 PID: 250 Comm: dbus-daemon Not tainted 3.14.51-03479-gf50bdf4 #1 task: a3ae61c0 ti: a08c8000 task.ti: a08c8000 PC is at retire_capture_urb+0x10/0x1f4 [snd_usb_audio] LR is at snd_complete_urb+0x140/0x1f0 [snd_usb_audio] pc : [<7f0eb22c>] lr : [<7f0e57fc>] psr: 200e0193 sp : a08c9c98 ip : a08c9ce8 fp : a08c9ce4 r10: 0000000a r9 : 00000102 r8 : 94cb3000 r7 : 94cb3000 r6 : 94d0f000 r5 : 94d0e8e8 r4 : 94d0e000 r3 : 7f0eb21c r2 : 00000000 r1 : 94cb3000 r0 : 00000000 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c5387d Table: 31d7c04a DAC: 00000015 Process dbus-daemon (pid: 250, stack limit = 0xa08c8238) Stack: (0xa08c9c98 to 0xa08ca000) ... Backtrace: [<7f0eb21c>] (retire_capture_urb [snd_usb_audio]) from [<7f0e57fc>] (snd_complete_urb+0x140/0x1f0 [snd_usb_audio]) [<7f0e56bc>] (snd_complete_urb [snd_usb_audio]) from [<80371118>] (__usb_hcd_giveback_urb+0x78/0xf4) [<803710a0>] (__usb_hcd_giveback_urb) from [<80371514>] (usb_giveback_urb_bh+0x8c/0xc0) [<80371488>] (usb_giveback_urb_bh) from [<80028e3c>] (tasklet_hi_action+0xc4/0x148) [<80028d78>] (tasklet_hi_action) from [<80028358>] (__do_softirq+0x190/0x380) [<800281c8>] (__do_softirq) from [<80028858>] (irq_exit+0x8c/0xfc) [<800287cc>] (irq_exit) from [<8000ea88>] (handle_IRQ+0x8c/0xc8) [<8000e9fc>] (handle_IRQ) from [<800085e8>] (gic_handle_irq+0xbc/0xf8) [<8000852c>] (gic_handle_irq) from [<80509044>] (__irq_svc+0x44/0x78) [<80508820>] (_raw_spin_unlock_irq) from [<8004b880>] (finish_task_switch+0x5c/0x100) [<8004b824>] (finish_task_switch) from [<805052f0>] (__schedule+0x48c/0x6d8) [<80504e64>] (__schedule) from [<805055d4>] (schedule+0x98/0x9c) [<8050553c>] (schedule) from [<800116c8>] (do_work_pending+0x30/0xd0) [<80011698>] (do_work_pending) from [<8000e160>] (work_pending+0xc/0x20) Code: e1a0c00d e92ddff0 e24cb004 e24dd024 (e5902008) Kernel panic - not syncing: Fatal exception in interrupt There is a race between retire_capture_urb() and stop_endpoints(). The latter is called at stopping the stream and it sets some endpoint fields to NULL. But its call is asynchronous, thus the pending complete callback might get called after these NULL clears, and it leads the NULL dereference like the above. The fix is to move the NULL clearance after the synchronization, i.e. wait_clear_urbs(). This is called at prepare and hw_free callbacks, so it's assured to be called before the restart of the stream or the release of the stream. Also, while we're at it, put the EP_FLAG_RUNNING flag check at the beginning of snd_complete_urb() to skip the pending complete after the stream is stopped. Fixes: b2eb950de2f0 ("ALSA: usb-audio: stop both data and sync...") Reported-by: Jiada Wang Reported-by: Mark Craske Cc: Signed-off-by: Takashi Iwai --- sound/usb/endpoint.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index c470251cea4b..f3fce9abece9 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -384,6 +384,9 @@ static void snd_complete_urb(struct urb *urb) if (unlikely(atomic_read(&ep->chip->shutdown))) goto exit_clear; + if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags))) + goto exit_clear; + if (usb_pipeout(ep->pipe)) { retire_outbound_urb(ep, ctx); /* can be stopped during retire callback */ @@ -534,6 +537,11 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) alive, ep->ep_num); clear_bit(EP_FLAG_STOPPING, &ep->flags); + ep->data_subs = NULL; + ep->sync_slave = NULL; + ep->retire_data_urb = NULL; + ep->prepare_data_urb = NULL; + return 0; } @@ -1006,10 +1014,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) if (--ep->use_count == 0) { deactivate_urbs(ep, false); - ep->data_subs = NULL; - ep->sync_slave = NULL; - ep->retire_data_urb = NULL; - ep->prepare_data_urb = NULL; set_bit(EP_FLAG_STOPPING, &ep->flags); } } -- 2.11.0