Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757774AbcK3JBG (ORCPT ); Wed, 30 Nov 2016 04:01:06 -0500 Received: from mx2.suse.de ([195.135.220.15]:57057 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755115AbcK3JA5 (ORCPT ); Wed, 30 Nov 2016 04:00:57 -0500 Date: Wed, 30 Nov 2016 10:00:55 +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: <20161130075923.15205-4-jiada_wang@mentor.com> References: <20161130075923.15205-1-jiada_wang@mentor.com> <20161130075923.15205-4-jiada_wang@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/24.5 (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: 4429 Lines: 111 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. > However, additional races may still happen in close+hw_free against > retire, as there pointers may be freed in addition. For example, while > in retire_capture_urb() runtime->dma_area might be freed/nulled. This shouldn't be a problem, as stop_endpoints() waits until all active URBS get killed. thanks, Takashi --- 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); } }