Commit 16200948 authored Dec 05, 2016 by Takashi Iwai

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: b2eb950d ("ALSA: usb-audio: stop both data and sync...")
Reported-by: Jiada Wang <jiada_wang@mentor.com>
Reported-by: Mark Craske <Mark_Craske@mentor.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

parent b5337cfe

sound/usb/endpoint.c

+8 −4

Original line number	Diff line number	Diff line
		@@ -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);
		}
		}

Admin message