Re: sound: another deadlock in snd_seq_pool_done

[Dmitry Vyukov]

On Tue, Mar 21, 2017 at 1:23 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> On Sat, 04 Mar 2017 17:31:21 +0100,
> Dmitry Vyukov wrote:
>>
>> Hello,
>>
>> The following program creates processes deadlocked in snd_seq_pool_done:
>>
>> https://gist.githubusercontent.com/dvyukov/97efc9cb6d63b1b2c7b737b82cc8b0d6/raw/3546b133ae0b2d3e1190ae7c1f4e240ce7ce132e/gistfile1.txt
>>
>> After few seconds I get:
>>
>> # ps afxu | grep a.out
>> root      8660  2.0  0.0      0     0 pts/0    Zl   16:27   0:00
>> [a.out] <defunct>
>>
>> # kill -9 8660
>>
>> # cat /proc/8660/status
>> Name: a.out
>> State: Z (zombie)
>> Tgid: 8660
>> Ngid: 0
>> Pid: 8660
>> PPid: 1
>> TracerPid: 0
>> Uid: 0 0 0 0
>> Gid: 0 0 0 0
>> FDSize: 0
>> Groups: 0
>> NStgid: 8660
>> NSpid: 8660
>> NSpgid: 8660
>> NSsid: 2971
>> Threads: 2
>> SigQ: 1/3304
>> SigPnd: 0000000000000000
>> ShdPnd: 0000000000000100
>> SigBlk: 0000000000000000
>> SigIgn: 0000000180000000
>> SigCgt: 0000000000000440
>> CapInh: 0000000000000000
>> CapPrm: 0000003fffffffff
>> CapEff: 0000003fffffffff
>> CapBnd: 0000003fffffffff
>> CapAmb: 0000000000000000
>> NoNewPrivs: 0
>> Seccomp: 0
>> Cpus_allowed: f
>> Cpus_allowed_list: 0-3
>> Mems_allowed: 00000000,00000001
>> Mems_allowed_list: 0
>> voluntary_ctxt_switches: 12
>> nonvoluntary_ctxt_switches: 0
>>
>> # cat /proc/8660/task/*/stack
>> [<ffffffff835406db>] snd_seq_pool_done+0x31b/0x620
>> sound/core/seq/seq_memory.c:436
>> [<ffffffff8353a11e>] snd_seq_ioctl_set_client_pool+0x1ae/0x600
>> sound/core/seq/seq_clientmgr.c:1836
>> [<ffffffff835382ba>] snd_seq_ioctl+0x2da/0x4d0
>> sound/core/seq/seq_clientmgr.c:2130
>> [<ffffffff81aced2f>] vfs_ioctl fs/ioctl.c:45 [inline]
>> [<ffffffff81aced2f>] do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:685
>> [<ffffffff81ad038f>] SYSC_ioctl fs/ioctl.c:700 [inline]
>> [<ffffffff81ad038f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:691
>> [<ffffffff8457dc41>] entry_SYSCALL_64_fastpath+0x1f/0xc2
>
> Now I've been trying to reproduce the hang, alas it didn't happen on
> my VM by some reason.
>
> In anyway, below is an untested fix for possible races against
> snd_seq_pool_done() and cell insertions.  Could you check whether it
> cures your issue?


I reproduced it again within 10 second. Then restarted kernel with
this patch and now it runs for 10 minutes without any hanged
processes.

Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>

Thanks!


> ---
> diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
> index 4c935202ce23..f3b1d7f50b81 100644
> --- a/sound/core/seq/seq_clientmgr.c
> +++ b/sound/core/seq/seq_clientmgr.c
> @@ -1832,6 +1832,7 @@ static int snd_seq_ioctl_set_client_pool(struct snd_seq_client *client,
>              info->output_pool != client->pool->size)) {
>                 if (snd_seq_write_pool_allocated(client)) {
>                         /* remove all existing cells */
> +                       snd_seq_pool_mark_closing(client->pool);
>                         snd_seq_queue_client_leave_cells(client->number);
>                         snd_seq_pool_done(client->pool);
>                 }
> diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c
> index 448efd4e980e..33980d1c8037 100644
> --- a/sound/core/seq/seq_fifo.c
> +++ b/sound/core/seq/seq_fifo.c
> @@ -72,6 +72,9 @@ void snd_seq_fifo_delete(struct snd_seq_fifo **fifo)
>                 return;
>         *fifo = NULL;
>
> +       if (f->pool)
> +               snd_seq_pool_mark_closing(f->pool);
> +
>         snd_seq_fifo_clear(f);
>
>         /* wake up clients if any */
> diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c
> index 1a1acf3ddda4..d4c61ec9be13 100644
> --- a/sound/core/seq/seq_memory.c
> +++ b/sound/core/seq/seq_memory.c
> @@ -415,6 +415,18 @@ int snd_seq_pool_init(struct snd_seq_pool *pool)
>         return 0;
>  }
>
> +/* refuse the further insertion to the pool */
> +void snd_seq_pool_mark_closing(struct snd_seq_pool *pool)
> +{
> +       unsigned long flags;
> +
> +       if (snd_BUG_ON(!pool))
> +               return;
> +       spin_lock_irqsave(&pool->lock, flags);
> +       pool->closing = 1;
> +       spin_unlock_irqrestore(&pool->lock, flags);
> +}
> +
>  /* remove events */
>  int snd_seq_pool_done(struct snd_seq_pool *pool)
>  {
> @@ -425,10 +437,6 @@ int snd_seq_pool_done(struct snd_seq_pool *pool)
>                 return -EINVAL;
>
>         /* wait for closing all threads */
> -       spin_lock_irqsave(&pool->lock, flags);
> -       pool->closing = 1;
> -       spin_unlock_irqrestore(&pool->lock, flags);
> -
>         if (waitqueue_active(&pool->output_sleep))
>                 wake_up(&pool->output_sleep);
>
> @@ -485,6 +493,7 @@ int snd_seq_pool_delete(struct snd_seq_pool **ppool)
>         *ppool = NULL;
>         if (pool == NULL)
>                 return 0;
> +       snd_seq_pool_mark_closing(pool);
>         snd_seq_pool_done(pool);
>         kfree(pool);
>         return 0;
> diff --git a/sound/core/seq/seq_memory.h b/sound/core/seq/seq_memory.h
> index 4a2ec779b8a7..32f959c17786 100644
> --- a/sound/core/seq/seq_memory.h
> +++ b/sound/core/seq/seq_memory.h
> @@ -84,6 +84,7 @@ static inline int snd_seq_total_cells(struct snd_seq_pool *pool)
>  int snd_seq_pool_init(struct snd_seq_pool *pool);
>
>  /* done pool - free events */
> +void snd_seq_pool_mark_closing(struct snd_seq_pool *pool);
>  int snd_seq_pool_done(struct snd_seq_pool *pool);
>
>  /* create pool */