Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1346576 > unrolled thread

sound: uninterruptible hang in snd_seq_oss_writeq_sync

Started byDmitry Vyukov <dvyukov@google.com>
First post2016-03-01 13:40 +0100
Last post2016-03-02 10:30 +0100
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

Contents

  sound: uninterruptible hang in snd_seq_oss_writeq_sync Dmitry Vyukov <dvyukov@google.com> - 2016-03-01 13:40 +0100
    Re: sound: uninterruptible hang in snd_seq_oss_writeq_sync Takashi Iwai <tiwai@suse.de> - 2016-03-01 15:50 +0100
      Re: sound: uninterruptible hang in snd_seq_oss_writeq_sync Dmitry Vyukov <dvyukov@google.com> - 2016-03-01 16:10 +0100
        Re: sound: uninterruptible hang in snd_seq_oss_writeq_sync Takashi Iwai <tiwai@suse.de> - 2016-03-01 16:40 +0100
          Re: sound: uninterruptible hang in snd_seq_oss_writeq_sync Dmitry Vyukov <dvyukov@google.com> - 2016-03-01 16:50 +0100
            Re: sound: uninterruptible hang in snd_seq_oss_writeq_sync Takashi Iwai <tiwai@suse.de> - 2016-03-01 18:50 +0100
              Re: sound: uninterruptible hang in snd_seq_oss_writeq_sync Dmitry Vyukov <dvyukov@google.com> - 2016-03-02 10:30 +0100

#1346576 — sound: uninterruptible hang in snd_seq_oss_writeq_sync

FromDmitry Vyukov <dvyukov@google.com>
Date2016-03-01 13:40 +0100
Subjectsound: uninterruptible hang in snd_seq_oss_writeq_sync
Message-ID<r7RKO-Hz-13@gated-at.bofh.it>
Hello,

The following program creates an unkillable process:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <pthread.h>
#include <stdint.h>
#include <string.h>
#include <sys/syscall.h>
#include <unistd.h>

#ifndef SYS_mmap
#define SYS_mmap 9
#endif
#ifndef SYS_syz_open_dev
#define SYS_syz_open_dev 1000001
#endif
#ifndef SYS_write
#define SYS_write 1
#endif
#ifndef SYS_read
#define SYS_read 0
#endif
#ifndef SYS_sync_file_range
#define SYS_sync_file_range 277
#endif

long r[61];

int main()
{
  memset(r, -1, sizeof(r));
  r[0] = syscall(SYS_mmap, 0x20000000ul, 0x5e000ul, 0x3ul, 0x32ul,
                 0xfffffffffffffffful, 0x0ul);
  r[2] = syscall(SYS_open, "/dev/sequencer", 0x202ul, 0, 0, 0);
  *(uint8_t*)0x20057000 = (uint8_t)0x7;
  *(uint8_t*)0x20057001 = (uint8_t)0x6;
  *(uint8_t*)0x20057002 = (uint8_t)0x3;
  *(uint8_t*)0x20057003 = (uint8_t)0x99be;
  *(uint32_t*)0x2005700c = (uint32_t)0x4;
  *(uint8_t*)0x20057010 = (uint8_t)0x5;
  *(uint8_t*)0x20057011 = (uint8_t)0xffffffffffffffff;
  *(uint8_t*)0x20057012 = (uint8_t)0x100;
  *(uint8_t*)0x20057013 = (uint8_t)0x1;
  *(uint8_t*)0x20057024 = (uint8_t)0x8;
  *(uint8_t*)0x20057025 = (uint8_t)0xabe4;
  *(uint16_t*)0x20057026 = (uint16_t)0x3;
  *(uint64_t*)0x20057028 = (uint64_t)0x2005055c;
  *(uint8_t*)0x20057030 = (uint8_t)0xffffffffffffffe0;
  *(uint8_t*)0x20057031 = (uint8_t)0xffffffffffffffb8;
  *(uint8_t*)0x20057032 = (uint8_t)0x8;
  *(uint8_t*)0x20057033 = (uint8_t)0x7;
  *(uint32_t*)0x2005703c = (uint32_t)0x5;
  *(uint8_t*)0x20057040 = (uint8_t)0x5;
  *(uint8_t*)0x20057041 = (uint8_t)0x401;
  *(uint8_t*)0x20057042 = (uint8_t)0x8;
  *(uint8_t*)0x20057043 = (uint8_t)0x4;
  *(uint8_t*)0x2005704c = (uint8_t)0x0;
  *(uint8_t*)0x2005704d = (uint8_t)0x3;
  *(uint8_t*)0x2005704e = (uint8_t)0x200;
  *(uint8_t*)0x2005704f = (uint8_t)0xee;
  *(uint8_t*)0x20057050 = (uint8_t)0x2;
  *(uint8_t*)0x20057051 = (uint8_t)0x8;
  *(uint8_t*)0x20057052 = (uint8_t)0x800;
  *(uint8_t*)0x20057053 = (uint8_t)0xfffffffffffff000;
  *(uint64_t*)0x20057068 = (uint64_t)0x77359400;
  *(uint64_t*)0x20057070 = (uint64_t)0x0;
  *(uint8_t*)0x20057078 = (uint8_t)0x10000;
  *(uint8_t*)0x20057079 = (uint8_t)0x946;
  *(uint8_t*)0x2005707a = (uint8_t)0x1;
  *(uint8_t*)0x2005707b = (uint8_t)0x1ff;
  *(uint8_t*)0x2005708c = (uint8_t)0x3;
  *(uint32_t*)0x20057090 = (uint32_t)0x7;
  *(uint32_t*)0x20057094 = (uint32_t)0x1;
  *(uint8_t*)0x20057098 = (uint8_t)0x2;
  *(uint8_t*)0x20057099 = (uint8_t)0x7ff;
  *(uint8_t*)0x2005709a = (uint8_t)0x8;
  *(uint8_t*)0x2005709b = (uint8_t)0xdd8;
  *(uint64_t*)0x200570b0 = (uint64_t)0x1;
  *(uint64_t*)0x200570b8 = (uint64_t)0x0;
  *(uint8_t*)0x200570c0 = (uint8_t)0x9;
  *(uint8_t*)0x200570c1 = (uint8_t)0x3;
  *(uint8_t*)0x200570c2 = (uint8_t)0x7ff;
  *(uint8_t*)0x200570c3 = (uint8_t)0x8;
  *(uint32_t*)0x200570d4 = (uint32_t)0xec;
  *(uint64_t*)0x200570d8 = (uint64_t)0x20053f14;
  memcpy((void*)0x2005055c,
         "\xa4\x62\xd2\xb0\x88\xe0\x2b\xbd\xea\xa1\xa5\xf7\x7b\x89\xca"
         "\xb0\x43\xca\x19\x6d\x70\xb6\xa6\xcd\x30\xaa\x98\x07\xb3\xf3"
         "\xea\xf8\x44\x19\xc8\x83\xbc\xb7\x33\xd1\xd0\x5e\x8f\x34\x12"
         "\x82\xb4\xf8\xbd\x02\x63\x0b\xe7\x45\x23\x40\xa4\x51\x40\x23"
         "\x01\xcc\x57\x89\x40\x9c\xc1\x29\x36\xe2\xc0\x70\xe5\xda\xc9"
         "\x1f\x40\x25\xe0\x74\x98\x07\x75\xe9\x5a\xfc\x0e\x10\x3f\xd9"
         "\x4b\xc3\xc2\x48\xfa\x18\xb8\xda\x49\x3e\xeb\x69\x2c\x4b\xd2"
         "\xb7\xac\xc6\x5a\x1c\xfd",
         111);
  memcpy((void*)0x20053f14,
         "\x1b\x4b\xa5\x94\xeb\xe1\xd9\x4b\x57\xf2\xeb\xab\xdd\xe5\xba"
         "\x24\xf2\xc8\x36\x90\x88\x21\x2c\xec\x8e\x5a\x0d\x24\x0f\x48"
         "\xcc\x0d\x3c\xab\x21\x8e\xfa\x8f\x0d\xcd\x8d\x14\x99\xc3\x64"
         "\xea\x7f\xf3\x88\xe9\xf7\x4f\x4c\x12\xe9\x0e\x23\xad\x73\x7d"
         "\xa3\x4c\x45\x3f\xb3\x48\xb7\x44\x89\x41\x20\x7b\x2c\xbd\xf2"
         "\x89\x16\x1f\xbd\x80\x24\x72\xa1\x8d\x4b\x13\x74\xb2\xf4\xdf"
         "\x49\xe0\x6c\x40\x0d\xf2\x7c\x8a\xbf\xf2\xb1\x62\xe0\x7e\x51"
         "\x9a\xd4\x60\x64\x49\x96\x4b\x83\x91\xe5\x51\x50\x94\x3c\x41"
         "\xf6\xd1\xef\x1f\x90\x5a\x48\xa5\xe2\xf8\xe3\xd9\xf9\x82\x5e"
         "\xa1\x5c\x85\x1c\xbe\x47\xa2\x29\xb7\x5a\xf4\x14\x24\x92\x11"
         "\x4b\x6b\x54\x95\x5e\x9f\x1b\xda\x02\xa3\x84\xb3\x2b\x07\x13"
         "\x18\x6b\x31\xb4\xc1\x04\x30\x5f\xa0\xa2\xe9\xe6\x50\x6f\xed"
         "\x15\x10\xf5\xda\xa1\xe5\x06\xaf\x28\xe0\x53\xea\x88\x49\xf8"
         "\x97\x52\x4b\xe8\xa2\xce\x9f\xc7\x47\x4f\xd6\xc0\xe2\xb7\xc1"
         "\x7f\xa9\x54\x3d\x11\xe4\x99\x28\x65\x39\xb1\xf3\xec\x48\x43"
         "\x58\x19\x12\xf6\x38\x5b\xee\xca\x33\x8e\x4d",
         236);
  r[56] = syscall(SYS_write, r[2], 0x20057000ul, 0x9cul, 0, 0, 0);
  memcpy((void*)0x2005bfa7, "\x2f\x64\x65\x76\x2f\x76\x68\x63\x69\x00",
         10);
  return 0;
}


The hang stack is:

[<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
sound/core/seq/oss/seq_oss_writeq.c:121
[<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160
sound/core/seq/oss/seq_oss_init.c:448
[<ffffffff852f8d04>] odev_release+0x44/0x80 sound/core/seq/oss/seq_oss.c:152
[<ffffffff817ccdc6>] __fput+0x236/0x780 fs/file_table.c:208
[<ffffffff817cd395>] ____fput+0x15/0x20 fs/file_table.c:244
[<ffffffff813b8db0>] task_work_run+0x170/0x210 kernel/task_work.c:115
[<     inline     >] exit_task_work include/linux/task_work.h:21
[<ffffffff81363ca0>] do_exit+0xaf0/0x2d20 kernel/exit.c:748
[<ffffffff81366048>] do_group_exit+0x108/0x330 kernel/exit.c:878
[<     inline     >] SYSC_exit_group kernel/exit.c:889
[<ffffffff8136628d>] SyS_exit_group+0x1d/0x20 kernel/exit.c:887
[<ffffffff866a3236>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185


On commit fc77dbd34c5c99bce46d40a2491937c3bcbd10af (4.5-rc6).

[toc] | [next] | [standalone]

#1346662

FromTakashi Iwai <tiwai@suse.de>
Date2016-03-01 15:50 +0100
Message-ID<r7TMB-1XE-9@gated-at.bofh.it>
In reply to#1346576
On Tue, 01 Mar 2016 13:33:27 +0100,
Dmitry Vyukov wrote:
> 
> Hello,
> 
> The following program creates an unkillable process:
.....
> The hang stack is:
> 
> [<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
> sound/core/seq/oss/seq_oss_writeq.c:121

This is 
	wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);

and this should return zero
	if (signal_pending(current))
		/* interrupted - return 0 to finish sync */
		q->sync_event_put = 0;
	if (! q->sync_event_put || q->sync_time >= time)
		return 0;
	return 1;

> [<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160

... and this loop should break:
		while (snd_seq_oss_writeq_sync(dp->writeq))
			;

So, I see no obvious error in the code, so far.

I'm running your test program now with 8 parallel runs, but I couldn't
reproduce it.  Any other specifics?


thanks,

Takashi

[toc] | [prev] | [next] | [standalone]

#1346680

FromDmitry Vyukov <dvyukov@google.com>
Date2016-03-01 16:10 +0100
Message-ID<r7U5X-2ju-3@gated-at.bofh.it>
In reply to#1346662
On Tue, Mar 1, 2016 at 3:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 01 Mar 2016 13:33:27 +0100,
> Dmitry Vyukov wrote:
>>
>> Hello,
>>
>> The following program creates an unkillable process:
> .....
>> The hang stack is:
>>
>> [<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
>> sound/core/seq/oss/seq_oss_writeq.c:121
>
> This is
>         wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
>
> and this should return zero
>         if (signal_pending(current))
>                 /* interrupted - return 0 to finish sync */
>                 q->sync_event_put = 0;
>         if (! q->sync_event_put || q->sync_time >= time)
>                 return 0;
>         return 1;
>
>> [<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160
>
> ... and this loop should break:
>                 while (snd_seq_oss_writeq_sync(dp->writeq))
>                         ;
>
> So, I see no obvious error in the code, so far.
>
> I'm running your test program now with 8 parallel runs, but I couldn't
> reproduce it.  Any other specifics?
>
>
> thanks,
>
> Takashi

[toc] | [prev] | [next] | [standalone]

#1346704

FromTakashi Iwai <tiwai@suse.de>
Date2016-03-01 16:40 +0100
Message-ID<r7Uz0-2wf-19@gated-at.bofh.it>
In reply to#1346680
On Tue, 01 Mar 2016 16:04:43 +0100,
Dmitry Vyukov wrote:
> 
> On Tue, Mar 1, 2016 at 4:01 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Tue, Mar 1, 2016 at 3:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> On Tue, 01 Mar 2016 13:33:27 +0100,
> >> Dmitry Vyukov wrote:
> >>>
> >>> Hello,
> >>>
> >>> The following program creates an unkillable process:
> >> .....
> >>> The hang stack is:
> >>>
> >>> [<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
> >>> sound/core/seq/oss/seq_oss_writeq.c:121
> >>
> >> This is
> >>         wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
> >>
> >> and this should return zero
> >>         if (signal_pending(current))
> >>                 /* interrupted - return 0 to finish sync */
> >>                 q->sync_event_put = 0;
> >>         if (! q->sync_event_put || q->sync_time >= time)
> >>                 return 0;
> >>         return 1;
> >>
> >>> [<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160
> >>
> >> ... and this loop should break:
> >>                 while (snd_seq_oss_writeq_sync(dp->writeq))
> >>                         ;
> >>
> >> So, I see no obvious error in the code, so far.
> >>
> >> I'm running your test program now with 8 parallel runs, but I couldn't
> >> reproduce it.  Any other specifics?
> 
> 
> Hummm.. for me this process hangs every time, even if I just run it
> once from console.
> I've now retested in on clean commit
> fc77dbd34c5c99bce46d40a2491937c3bcbd10af (without KASAN/KCOV and with
> release gcc) and also got the same hang.
> I can only think of a different config. I've attached mine, please try with it.
> 
> Are signals delivered if we are already in process of dying?
> When I kill -9 this process it does not react, so presumably wait is
> not unblocked...

Good point.

But above all, it doesn't make much sense to loop this sync call.
When wait_event*() returns, it should go out.  So, the patch like
below should be good enough and fix your issue, too.


Takashi

---
diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c
index 1f6788a18444..4014a5469d1a 100644
--- a/sound/core/seq/oss/seq_oss_writeq.c
+++ b/sound/core/seq/oss/seq_oss_writeq.c
@@ -119,12 +119,7 @@ snd_seq_oss_writeq_sync(struct seq_oss_writeq *q)
 	}
 
 	wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
-	if (signal_pending(current))
-		/* interrupted - return 0 to finish sync */
-		q->sync_event_put = 0;
-	if (! q->sync_event_put || q->sync_time >= time)
-		return 0;
-	return 1;
+	return 0;
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1346709

FromDmitry Vyukov <dvyukov@google.com>
Date2016-03-01 16:50 +0100
Message-ID<r7UIG-2Bg-5@gated-at.bofh.it>
In reply to#1346704
On Tue, Mar 1, 2016 at 4:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 01 Mar 2016 16:04:43 +0100,
> Dmitry Vyukov wrote:
>>
>> On Tue, Mar 1, 2016 at 4:01 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> > On Tue, Mar 1, 2016 at 3:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> On Tue, 01 Mar 2016 13:33:27 +0100,
>> >> Dmitry Vyukov wrote:
>> >>>
>> >>> Hello,
>> >>>
>> >>> The following program creates an unkillable process:
>> >> .....
>> >>> The hang stack is:
>> >>>
>> >>> [<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
>> >>> sound/core/seq/oss/seq_oss_writeq.c:121
>> >>
>> >> This is
>> >>         wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
>> >>
>> >> and this should return zero
>> >>         if (signal_pending(current))
>> >>                 /* interrupted - return 0 to finish sync */
>> >>                 q->sync_event_put = 0;
>> >>         if (! q->sync_event_put || q->sync_time >= time)
>> >>                 return 0;
>> >>         return 1;
>> >>
>> >>> [<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160
>> >>
>> >> ... and this loop should break:
>> >>                 while (snd_seq_oss_writeq_sync(dp->writeq))
>> >>                         ;
>> >>
>> >> So, I see no obvious error in the code, so far.
>> >>
>> >> I'm running your test program now with 8 parallel runs, but I couldn't
>> >> reproduce it.  Any other specifics?
>>
>>
>> Hummm.. for me this process hangs every time, even if I just run it
>> once from console.
>> I've now retested in on clean commit
>> fc77dbd34c5c99bce46d40a2491937c3bcbd10af (without KASAN/KCOV and with
>> release gcc) and also got the same hang.
>> I can only think of a different config. I've attached mine, please try with it.
>>
>> Are signals delivered if we are already in process of dying?
>> When I kill -9 this process it does not react, so presumably wait is
>> not unblocked...
>
> Good point.
>
> But above all, it doesn't make much sense to loop this sync call.
> When wait_event*() returns, it should go out.  So, the patch like
> below should be good enough and fix your issue, too.



The patch fixes the hang for me. There is a second delay at exit, but
otherwise the process exits fine.

Tested-by: Dmitry Vyukov <dvyukov@google.com>

Thanks for quick fix!

> ---
> diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c
> index 1f6788a18444..4014a5469d1a 100644
> --- a/sound/core/seq/oss/seq_oss_writeq.c
> +++ b/sound/core/seq/oss/seq_oss_writeq.c
> @@ -119,12 +119,7 @@ snd_seq_oss_writeq_sync(struct seq_oss_writeq *q)
>         }
>
>         wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
> -       if (signal_pending(current))
> -               /* interrupted - return 0 to finish sync */
> -               q->sync_event_put = 0;
> -       if (! q->sync_event_put || q->sync_time >= time)
> -               return 0;
> -       return 1;
> +       return 0;
>  }
>
>  /*

[toc] | [prev] | [next] | [standalone]

#1346814

FromTakashi Iwai <tiwai@suse.de>
Date2016-03-01 18:50 +0100
Message-ID<r7WAO-3OJ-29@gated-at.bofh.it>
In reply to#1346709
On Tue, 01 Mar 2016 16:45:22 +0100,
Dmitry Vyukov wrote:
> 
> On Tue, Mar 1, 2016 at 4:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 01 Mar 2016 16:04:43 +0100,
> > Dmitry Vyukov wrote:
> >>
> >> On Tue, Mar 1, 2016 at 4:01 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> >> > On Tue, Mar 1, 2016 at 3:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >> On Tue, 01 Mar 2016 13:33:27 +0100,
> >> >> Dmitry Vyukov wrote:
> >> >>>
> >> >>> Hello,
> >> >>>
> >> >>> The following program creates an unkillable process:
> >> >> .....
> >> >>> The hang stack is:
> >> >>>
> >> >>> [<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
> >> >>> sound/core/seq/oss/seq_oss_writeq.c:121
> >> >>
> >> >> This is
> >> >>         wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
> >> >>
> >> >> and this should return zero
> >> >>         if (signal_pending(current))
> >> >>                 /* interrupted - return 0 to finish sync */
> >> >>                 q->sync_event_put = 0;
> >> >>         if (! q->sync_event_put || q->sync_time >= time)
> >> >>                 return 0;
> >> >>         return 1;
> >> >>
> >> >>> [<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160
> >> >>
> >> >> ... and this loop should break:
> >> >>                 while (snd_seq_oss_writeq_sync(dp->writeq))
> >> >>                         ;
> >> >>
> >> >> So, I see no obvious error in the code, so far.
> >> >>
> >> >> I'm running your test program now with 8 parallel runs, but I couldn't
> >> >> reproduce it.  Any other specifics?
> >>
> >>
> >> Hummm.. for me this process hangs every time, even if I just run it
> >> once from console.
> >> I've now retested in on clean commit
> >> fc77dbd34c5c99bce46d40a2491937c3bcbd10af (without KASAN/KCOV and with
> >> release gcc) and also got the same hang.
> >> I can only think of a different config. I've attached mine, please try with it.
> >>
> >> Are signals delivered if we are already in process of dying?
> >> When I kill -9 this process it does not react, so presumably wait is
> >> not unblocked...
> >
> > Good point.
> >
> > But above all, it doesn't make much sense to loop this sync call.
> > When wait_event*() returns, it should go out.  So, the patch like
> > below should be good enough and fix your issue, too.
> 
> 
> 
> The patch fixes the hang for me. There is a second delay at exit, but
> otherwise the process exits fine.
> 
> Tested-by: Dmitry Vyukov <dvyukov@google.com>
> 
> Thanks for quick fix!

OK, I got the same result finally on my VM, too.  I had to disable
INPUT_PCSPKR that conflicts with SND_PCSP.

I reconsidered about this problem again, and concluded that the
currently implemented drain behavior is simply superfluous.  So, the
fix is just to remove all these relevant calls.  The patch is attached
below.  Try this one instead of the previous one.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: seq: oss: Don't drain at closing a client

The OSS sequencer client tries to drain the pending events at
releasing.  Unfortunately, as spotted by syzkaller fuzzer, this may
lead to an unkillable process state when the event has been queued at
the far future.  Since the process being released can't be signaled
any longer, it remains and waits for the echo-back event in that far
future.

Back to history, the draining feature was implemented at the time we
misinterpreted POSIX definition for blocking file operation.
Actually, such a behavior is superfluous at release, and we should
just release the device as is instead of keeping it up forever.

This patch just removes the draining call that may block the release
for too long time unexpectedly.

BugLink: http://lkml.kernel.org/r/CACT4Y+Y4kD-aBGj37rf-xBw9bH3GMU6P+MYg4W1e-s-paVD2pg@mail.gmail.com
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/seq/oss/seq_oss.c        |  2 --
 sound/core/seq/oss/seq_oss_device.h |  1 -
 sound/core/seq/oss/seq_oss_init.c   | 16 ----------------
 3 files changed, 19 deletions(-)

diff --git a/sound/core/seq/oss/seq_oss.c b/sound/core/seq/oss/seq_oss.c
index 8db156b207f1..8cdf489df80e 100644
--- a/sound/core/seq/oss/seq_oss.c
+++ b/sound/core/seq/oss/seq_oss.c
@@ -149,8 +149,6 @@ odev_release(struct inode *inode, struct file *file)
 	if ((dp = file->private_data) == NULL)
 		return 0;
 
-	snd_seq_oss_drain_write(dp);
-
 	mutex_lock(&register_mutex);
 	snd_seq_oss_release(dp);
 	mutex_unlock(&register_mutex);
diff --git a/sound/core/seq/oss/seq_oss_device.h b/sound/core/seq/oss/seq_oss_device.h
index b43924325249..d7b4d016b547 100644
--- a/sound/core/seq/oss/seq_oss_device.h
+++ b/sound/core/seq/oss/seq_oss_device.h
@@ -127,7 +127,6 @@ int snd_seq_oss_write(struct seq_oss_devinfo *dp, const char __user *buf, int co
 unsigned int snd_seq_oss_poll(struct seq_oss_devinfo *dp, struct file *file, poll_table * wait);
 
 void snd_seq_oss_reset(struct seq_oss_devinfo *dp);
-void snd_seq_oss_drain_write(struct seq_oss_devinfo *dp);
 
 /* */
 void snd_seq_oss_process_queue(struct seq_oss_devinfo *dp, abstime_t time);
diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
index 6779e82b46dd..92c96a95a903 100644
--- a/sound/core/seq/oss/seq_oss_init.c
+++ b/sound/core/seq/oss/seq_oss_init.c
@@ -436,22 +436,6 @@ snd_seq_oss_release(struct seq_oss_devinfo *dp)
 
 
 /*
- * Wait until the queue is empty (if we don't have nonblock)
- */
-void
-snd_seq_oss_drain_write(struct seq_oss_devinfo *dp)
-{
-	if (! dp->timer->running)
-		return;
-	if (is_write_mode(dp->file_mode) && !is_nonblock_mode(dp->file_mode) &&
-	    dp->writeq) {
-		while (snd_seq_oss_writeq_sync(dp->writeq))
-			;
-	}
-}
-
-
-/*
  * reset sequencer devices
  */
 void
-- 
2.7.2

[toc] | [prev] | [next] | [standalone]

#1347878

FromDmitry Vyukov <dvyukov@google.com>
Date2016-03-02 10:30 +0100
Message-ID<r8bgu-5FN-13@gated-at.bofh.it>
In reply to#1346814
On Tue, Mar 1, 2016 at 6:43 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 01 Mar 2016 16:45:22 +0100,
> Dmitry Vyukov wrote:
>>
>> On Tue, Mar 1, 2016 at 4:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > On Tue, 01 Mar 2016 16:04:43 +0100,
>> > Dmitry Vyukov wrote:
>> >>
>> >> On Tue, Mar 1, 2016 at 4:01 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> >> > On Tue, Mar 1, 2016 at 3:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> >> On Tue, 01 Mar 2016 13:33:27 +0100,
>> >> >> Dmitry Vyukov wrote:
>> >> >>>
>> >> >>> Hello,
>> >> >>>
>> >> >>> The following program creates an unkillable process:
>> >> >> .....
>> >> >>> The hang stack is:
>> >> >>>
>> >> >>> [<ffffffff85309f77>] snd_seq_oss_writeq_sync+0x327/0x790
>> >> >>> sound/core/seq/oss/seq_oss_writeq.c:121
>> >> >>
>> >> >> This is
>> >> >>         wait_event_interruptible_timeout(q->sync_sleep, ! q->sync_event_put, HZ);
>> >> >>
>> >> >> and this should return zero
>> >> >>         if (signal_pending(current))
>> >> >>                 /* interrupted - return 0 to finish sync */
>> >> >>                 q->sync_event_put = 0;
>> >> >>         if (! q->sync_event_put || q->sync_time >= time)
>> >> >>                 return 0;
>> >> >>         return 1;
>> >> >>
>> >> >>> [<ffffffff852fa353>] snd_seq_oss_drain_write+0x113/0x160
>> >> >>
>> >> >> ... and this loop should break:
>> >> >>                 while (snd_seq_oss_writeq_sync(dp->writeq))
>> >> >>                         ;
>> >> >>
>> >> >> So, I see no obvious error in the code, so far.
>> >> >>
>> >> >> I'm running your test program now with 8 parallel runs, but I couldn't
>> >> >> reproduce it.  Any other specifics?
>> >>
>> >>
>> >> Hummm.. for me this process hangs every time, even if I just run it
>> >> once from console.
>> >> I've now retested in on clean commit
>> >> fc77dbd34c5c99bce46d40a2491937c3bcbd10af (without KASAN/KCOV and with
>> >> release gcc) and also got the same hang.
>> >> I can only think of a different config. I've attached mine, please try with it.
>> >>
>> >> Are signals delivered if we are already in process of dying?
>> >> When I kill -9 this process it does not react, so presumably wait is
>> >> not unblocked...
>> >
>> > Good point.
>> >
>> > But above all, it doesn't make much sense to loop this sync call.
>> > When wait_event*() returns, it should go out.  So, the patch like
>> > below should be good enough and fix your issue, too.
>>
>>
>>
>> The patch fixes the hang for me. There is a second delay at exit, but
>> otherwise the process exits fine.
>>
>> Tested-by: Dmitry Vyukov <dvyukov@google.com>
>>
>> Thanks for quick fix!
>
> OK, I got the same result finally on my VM, too.  I had to disable
> INPUT_PCSPKR that conflicts with SND_PCSP.
>
> I reconsidered about this problem again, and concluded that the
> currently implemented drain behavior is simply superfluous.  So, the
> fix is just to remove all these relevant calls.  The patch is attached
> below.  Try this one instead of the previous one.

Replaced the previous fix with this one.


> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: seq: oss: Don't drain at closing a client
>
> The OSS sequencer client tries to drain the pending events at
> releasing.  Unfortunately, as spotted by syzkaller fuzzer, this may
> lead to an unkillable process state when the event has been queued at
> the far future.  Since the process being released can't be signaled
> any longer, it remains and waits for the echo-back event in that far
> future.
>
> Back to history, the draining feature was implemented at the time we
> misinterpreted POSIX definition for blocking file operation.
> Actually, such a behavior is superfluous at release, and we should
> just release the device as is instead of keeping it up forever.
>
> This patch just removes the draining call that may block the release
> for too long time unexpectedly.
>
> BugLink: http://lkml.kernel.org/r/CACT4Y+Y4kD-aBGj37rf-xBw9bH3GMU6P+MYg4W1e-s-paVD2pg@mail.gmail.com
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/core/seq/oss/seq_oss.c        |  2 --
>  sound/core/seq/oss/seq_oss_device.h |  1 -
>  sound/core/seq/oss/seq_oss_init.c   | 16 ----------------
>  3 files changed, 19 deletions(-)
>
> diff --git a/sound/core/seq/oss/seq_oss.c b/sound/core/seq/oss/seq_oss.c
> index 8db156b207f1..8cdf489df80e 100644
> --- a/sound/core/seq/oss/seq_oss.c
> +++ b/sound/core/seq/oss/seq_oss.c
> @@ -149,8 +149,6 @@ odev_release(struct inode *inode, struct file *file)
>         if ((dp = file->private_data) == NULL)
>                 return 0;
>
> -       snd_seq_oss_drain_write(dp);
> -
>         mutex_lock(&register_mutex);
>         snd_seq_oss_release(dp);
>         mutex_unlock(&register_mutex);
> diff --git a/sound/core/seq/oss/seq_oss_device.h b/sound/core/seq/oss/seq_oss_device.h
> index b43924325249..d7b4d016b547 100644
> --- a/sound/core/seq/oss/seq_oss_device.h
> +++ b/sound/core/seq/oss/seq_oss_device.h
> @@ -127,7 +127,6 @@ int snd_seq_oss_write(struct seq_oss_devinfo *dp, const char __user *buf, int co
>  unsigned int snd_seq_oss_poll(struct seq_oss_devinfo *dp, struct file *file, poll_table * wait);
>
>  void snd_seq_oss_reset(struct seq_oss_devinfo *dp);
> -void snd_seq_oss_drain_write(struct seq_oss_devinfo *dp);
>
>  /* */
>  void snd_seq_oss_process_queue(struct seq_oss_devinfo *dp, abstime_t time);
> diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
> index 6779e82b46dd..92c96a95a903 100644
> --- a/sound/core/seq/oss/seq_oss_init.c
> +++ b/sound/core/seq/oss/seq_oss_init.c
> @@ -436,22 +436,6 @@ snd_seq_oss_release(struct seq_oss_devinfo *dp)
>
>
>  /*
> - * Wait until the queue is empty (if we don't have nonblock)
> - */
> -void
> -snd_seq_oss_drain_write(struct seq_oss_devinfo *dp)
> -{
> -       if (! dp->timer->running)
> -               return;
> -       if (is_write_mode(dp->file_mode) && !is_nonblock_mode(dp->file_mode) &&
> -           dp->writeq) {
> -               while (snd_seq_oss_writeq_sync(dp->writeq))
> -                       ;
> -       }
> -}
> -
> -
> -/*
>   * reset sequencer devices
>   */
>  void
> --
> 2.7.2
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web