Re: sound: divide by 0 in snd_hrtimer_callback (or hang)

[Takashi Iwai]

On Thu, 08 Sep 2016 17:14:18 +0200,
Dmitry Vyukov wrote:
> 
> On Wed, Sep 7, 2016 at 5:00 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > On Tue, 06 Sep 2016 18:10:30 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Tue, 06 Sep 2016 16:06:08 +0200,
> >> Dmitry Vyukov wrote:
> >> >
> >> > > Do you really see the zero-division error with sticks=1 fix?  That's
> >> > > unexpected.
> >> >
> >> > Yes, just reproduced it again.
> >> > I am on 0693c28cfc8e25f18c25b65a8942c026f1854a3c of linux-next now, so
> >> > I already have:
> >> >     commit 6b760bb2c63a9e322c0e4a0b5daf335ad93d5a33
> >> >     ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE
> >> >
> >> > Compiled the program verbatim and run it using stress utility (http
> >> > s://godoc.org/golang.org/x/tools/cmd/stress namely just runs the
> >> > program in parallel loop) and instantly get the following crash.
> >>
> >> OK, I'll take a deeper look.
> >
> > The fix patch below seems working on my system.  Please give it a
> > try.
> 
> 
> Pulled this from linux-next.
> This should fix both division and stalls, right?

Yes, it looks so.  The latter one seems to be the side-effect of the
stuck by the first one.

> I will let you know if I see any of these again.

Thanks.


Takashi

> 
> 
> > -- 8< --
> > From: Takashi Iwai <tiwai@xxxxxxx>
> > Subject: [PATCH] ALSA: timer: Fix zero-division by continue of uninitialized
> >  instance
> >
> > When a user timer instance is continued without the explicit start
> > beforehand, the system gets eventually zero-division error like:
> >
> >   divide error: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
> >   CPU: 1 PID: 27320 Comm: syz-executor Not tainted 4.8.0-rc3-next-20160825+ #8
> >   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >    task: ffff88003c9b2280 task.stack: ffff880027280000
> >    RIP: 0010:[<ffffffff858e1a6c>]  [<     inline     >] ktime_divns include/linux/ktime.h:195
> >    RIP: 0010:[<ffffffff858e1a6c>]  [<ffffffff858e1a6c>] snd_hrtimer_callback+0x1bc/0x3c0 sound/core/hrtimer.c:62
> >   Call Trace:
> >    <IRQ>
> >    [<     inline     >] __run_hrtimer kernel/time/hrtimer.c:1238
> >    [<ffffffff81504335>] __hrtimer_run_queues+0x325/0xe70 kernel/time/hrtimer.c:1302
> >    [<ffffffff81506ceb>] hrtimer_interrupt+0x18b/0x420 kernel/time/hrtimer.c:1336
> >    [<ffffffff8126d8df>] local_apic_timer_interrupt+0x6f/0xe0 arch/x86/kernel/apic/apic.c:933
> >    [<ffffffff86e13056>] smp_apic_timer_interrupt+0x76/0xa0 arch/x86/kernel/apic/apic.c:957
> >    [<ffffffff86e1210c>] apic_timer_interrupt+0x8c/0xa0 arch/x86/entry/entry_64.S:487
> >    <EOI>
> >    .....
> >
> > Although a similar issue was spotted and a fix patch was merged in
> > commit [6b760bb2c63a: ALSA: timer: fix division by zero after
> > SNDRV_TIMER_IOCTL_CONTINUE], it seems covering only a part of
> > iceberg.
> >
> > In this patch, we fix the issue a bit more drastically.  Basically the
> > continue of an uninitialized timer is supposed to be a fresh start, so
> > we do it for user timers.  For the direct snd_timer_continue() call,
> > there is no way to pass the initial tick value, so we kick out for the
> > uninitialized case.
> >
> > Reported-by: Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > ---
> >  sound/core/timer.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/sound/core/timer.c b/sound/core/timer.c
> > index 2706061fc1ea..fc144f43faa6 100644
> > --- a/sound/core/timer.c
> > +++ b/sound/core/timer.c
> > @@ -35,6 +35,9 @@
> >  #include <sound/initval.h>
> >  #include <linux/kmod.h>
> >
> > +/* internal flags */
> > +#define SNDRV_TIMER_IFLG_PAUSED                0x00010000
> > +
> >  #if IS_ENABLED(CONFIG_SND_HRTIMER)
> >  #define DEFAULT_TIMER_LIMIT 4
> >  #else
> > @@ -539,6 +542,10 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
> >                 }
> >         }
> >         timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START);
> > +       if (stop)
> > +               timeri->flags &= ~SNDRV_TIMER_IFLG_PAUSED;
> > +       else
> > +               timeri->flags |= SNDRV_TIMER_IFLG_PAUSED;
> >         snd_timer_notify1(timeri, stop ? SNDRV_TIMER_EVENT_STOP :
> >                           SNDRV_TIMER_EVENT_CONTINUE);
> >   unlock:
> > @@ -600,6 +607,10 @@ int snd_timer_stop(struct snd_timer_instance *timeri)
> >   */
> >  int snd_timer_continue(struct snd_timer_instance *timeri)
> >  {
> > +       /* timer can continue only after pause */
> > +       if (!(timeri->flags & SNDRV_TIMER_IFLG_PAUSED))
> > +               return -EINVAL;
> > +
> >         if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE)
> >                 return snd_timer_start_slave(timeri, false);
> >         else
> > @@ -1831,6 +1842,9 @@ static int snd_timer_user_continue(struct file *file)
> >         tu = file->private_data;
> >         if (!tu->timeri)
> >                 return -EBADFD;
> > +       /* start timer instead of continue if it's not used before */
> > +       if (!(tu->timeri->flags & SNDRV_TIMER_IFLG_PAUSED))
> > +               return snd_timer_user_start(file);
> >         tu->timeri->lost = 0;
> >         return (err = snd_timer_continue(tu->timeri)) < 0 ? err : 0;
> >  }
> > --
> > 2.10.0
> >
>