Re: [PATCH 3/3 v3] ALSA: usb-audio: fix race in snd_usb_endpoint_stop

[Jiada Wang <jiada_wang@mentor.com>]

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<Mark_Craske@mentor.com>
>>
>> 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?

Thanks,
Jiada
>
>> 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);
>   	}
>   }