From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932984AbcDTKbH (ORCPT ); Wed, 20 Apr 2016 06:31:07 -0400 Received: from mx2.suse.de ([195.135.220.15]:56571 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932440AbcDTKbF (ORCPT ); Wed, 20 Apr 2016 06:31:05 -0400 Date: Wed, 20 Apr 2016 12:31:00 +0200 Message-ID: From: Takashi Iwai To: Dmitry Vyukov Cc: alsa-devel@alsa-project.org, Peter Zijlstra , Thomas Gleixner , Jaroslav Kysela , LKML , Alexander Potapenko , Kostya Serebryany , syzkaller , Sasha Levin Subject: Re: sound: use-after-free in snd_timer_interrupt In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: multipart/mixed; boundary="Multipart_Wed_Apr_20_12:31:00_2016-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --Multipart_Wed_Apr_20_12:31:00_2016-1 Content-Type: text/plain; charset=US-ASCII On Wed, 20 Apr 2016 10:08:55 +0200, Takashi Iwai wrote: > > On Wed, 20 Apr 2016 09:56:04 +0200, > Dmitry Vyukov wrote: > > > > On Sun, Apr 3, 2016 at 8:33 AM, Takashi Iwai wrote: > > >> >> It is not easily reproducible. I've hit several times while running > > >> >> fuzzer for a week. Here is one of the logs for the record: > > >> >> https://gist.githubusercontent.com/dvyukov/c84798ee55721563ecb537c4d51dc9f5/raw/f00b865a85877656f13b41917f7321730f140d35/gistfile1.txt > > >> > > > >> > There are a few more fixes in sound/core/timer.c since 4.5, and they > > >> > possibly already cover this. > > >> > > > >> > Please let me know if this is still seen on the upcoming 4.6-rc2. > > >> > > >> Hi Takashi, > > >> > > >> I've updated fuzzer to 05cf8077e54b20dddb756eaa26f3aeb5c38dd3cf (Apr > > >> 1) yesterday. Let's see if it still happens. > > >> > > >> Out of curiosity, how was the bug found? > > > > > > Well, I'm not entirely sure whether they really cover. It's just a > > > hope, as these are patches to close some possible races :) > > > > > > 9984d1b5835ca29fc7025186a891ee7398d21cc7 > > > ALSA: timer: Protect the whole snd_timer_close() with open race > > > f65e0d299807d8a11812845c972493c3f9a18e10 > > > ALSA: timer: Call notifier in the same spinlock > > > 4a07083ed613644c96c34a7dd2853dc5d7c70902 > > > ALSA: timer: Use mod_timer() for rearming the system timer > > > > > > Hi Takashi, > > > > I've hit it again on 806fdcce017dc98c4dbf8ed001750a0d7d2bb0af (Apr > > 14), all 3 commits are already in my tree. > > > > [ 343.222218] ------------[ cut here ]------------ > > [ 343.222218] WARNING: CPU: 3 PID: 7040 at kernel/time/hrtimer.c:837 > > hrtimer_forward+0x26a/0x3e0 > > This is a different warning. The previous was use-after-free, and > this is a warning about re-arming the queued hrtimer. > Maybe there is a slightly remaining race about hrtimer_start() and the > interrupt handler in snd-hrtimer. Could you check whether two patches below help anything? This should harden against the race between hrtimer callback and another start/stop calls. Takashi --Multipart_Wed_Apr_20_12:31:00_2016-1 Content-Type: application/octet-stream; type=patch Content-Disposition: attachment; filename="0001-ALSA-timer-Allow-backend-disabling-start-stop-from-h.patch" Content-Transfer-Encoding: 7bit >>From 18dee7aebf62375a224025ee0ee287c36784aa6d Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Wed, 20 Apr 2016 12:02:36 +0200 Subject: [PATCH 1/2] ALSA: timer: Allow backend disabling start/stop from handler Some timer backend doesn't particularly like (re)start / stop calls from its interrupt handler. For example, hrtimer can't stop properly with sync, and we still seem to have some open race. This patch introduced a new flag, SNDRV_TIMER_HW_RET_CTRL, so that the timer backend can specify whether snd_timer_interrupt() should call hw start() and hw.stop() callbacks or not. If the new flag is set, snd_timer_interrupt() won't call hw.start() and hw.stop() callbacks but return SNDRV_TIMER_RET_START and SNDRV_TIMER_RET_STOP, respectively. Signed-off-by: Takashi Iwai --- include/sound/timer.h | 12 +++++++++++- sound/core/timer.c | 24 ++++++++++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/include/sound/timer.h b/include/sound/timer.h index c4d76ff056c6..6ca6ed4169da 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -37,6 +37,7 @@ #define SNDRV_TIMER_HW_SLAVE 0x00000004 /* only slave timer (variable resolution) */ #define SNDRV_TIMER_HW_FIRST 0x00000008 /* first tick can be incomplete */ #define SNDRV_TIMER_HW_TASKLET 0x00000010 /* timer is called from tasklet */ +#define SNDRV_TIMER_HW_RET_CTRL 0x00000020 /* don't start/stop at irq handler */ #define SNDRV_TIMER_IFLG_SLAVE 0x00000001 #define SNDRV_TIMER_IFLG_RUNNING 0x00000002 @@ -50,6 +51,15 @@ #define SNDRV_TIMER_FLG_CHANGE 0x00000001 #define SNDRV_TIMER_FLG_RESCHED 0x00000002 /* need reschedule */ +/* return value from snd_timer_interrupt(); + * START and STOP are returned only when SNDRV_TIMER_HW_RET_CTRL is set + */ +enum { + SNDRV_TIMER_RET_NONE = 0, + SNDRV_TIMER_RET_START = 1, + SNDRV_TIMER_RET_STOP = 2, +}; + struct snd_timer; struct snd_timer_hardware { @@ -139,6 +149,6 @@ int snd_timer_stop(struct snd_timer_instance *timeri); int snd_timer_continue(struct snd_timer_instance *timeri); int snd_timer_pause(struct snd_timer_instance *timeri); -void snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left); +int snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left); #endif /* __SOUND_TIMER_H */ diff --git a/sound/core/timer.c b/sound/core/timer.c index 6469bedda2f3..c653c409d74d 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -683,19 +683,20 @@ static void snd_timer_tasklet(unsigned long arg) * ticks_left is usually equal to timer->sticks. * */ -void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) +int snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left) { struct snd_timer_instance *ti, *ts, *tmp; unsigned long resolution, ticks; struct list_head *p, *ack_list_head; unsigned long flags; int use_tasklet = 0; + int ret = 0; if (timer == NULL) - return; + return -ENODEV; if (timer->card && timer->card->shutdown) - return; + return -ENODEV; spin_lock_irqsave(&timer->lock, flags); @@ -747,17 +748,26 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) snd_timer_reschedule(timer, timer->sticks); if (timer->running) { if (timer->hw.flags & SNDRV_TIMER_HW_STOP) { - timer->hw.stop(timer); + if (timer->hw.flags & SNDRV_TIMER_HW_RET_CTRL) + ret = SNDRV_TIMER_RET_STOP; + else + timer->hw.stop(timer); timer->flags |= SNDRV_TIMER_FLG_CHANGE; } if (!(timer->hw.flags & SNDRV_TIMER_HW_AUTO) || (timer->flags & SNDRV_TIMER_FLG_CHANGE)) { /* restart timer */ timer->flags &= ~SNDRV_TIMER_FLG_CHANGE; - timer->hw.start(timer); + if (timer->hw.flags & SNDRV_TIMER_HW_RET_CTRL) + ret = SNDRV_TIMER_RET_START; + else + timer->hw.start(timer); } } else { - timer->hw.stop(timer); + if (timer->hw.flags & SNDRV_TIMER_HW_RET_CTRL) + ret = SNDRV_TIMER_RET_STOP; + else + timer->hw.stop(timer); } /* now process all fast callbacks */ @@ -785,6 +795,8 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) if (use_tasklet) tasklet_schedule(&timer->task_queue); + + return ret; } /* -- 2.8.1 --Multipart_Wed_Apr_20_12:31:00_2016-1 Content-Type: application/octet-stream; type=patch Content-Disposition: attachment; filename="0002-ALSA-hrtimer-Use-manual-start-stop-in-callback.patch" Content-Transfer-Encoding: 7bit >>From 360230be1a52e0f81470820cf2be4e7da50e25f8 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Wed, 20 Apr 2016 12:10:10 +0200 Subject: [PATCH 2/2] ALSA: hrtimer: Use manual start/stop in callback With the new SNDRV_TIMER_HW_RET_CTRL flag, hrtimer can manage the callback behavior more correctly. Now it gets a return value from snd_timer_interrupt() whether to reprogram or stop the timer, and it can choose the right return value. The biggest bonus by this change is that we can protect the whole interrupt with a spinlock against start/stop calls from another thread. The patch adds a spinlock and converts the former atomic flag into a normal bool flag. Signed-off-by: Takashi Iwai --- sound/core/hrtimer.c | 51 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c index 656d9a9032dc..8a1b51473fd5 100644 --- a/sound/core/hrtimer.c +++ b/sound/core/hrtimer.c @@ -38,7 +38,8 @@ static unsigned int resolution; struct snd_hrtimer { struct snd_timer *timer; struct hrtimer hrt; - atomic_t running; + spinlock_t lock; + bool running; }; static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt) @@ -46,16 +47,33 @@ static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt) struct snd_hrtimer *stime = container_of(hrt, struct snd_hrtimer, hrt); struct snd_timer *t = stime->timer; unsigned long oruns; + int irq_ret; + enum hrtimer_restart ret = HRTIMER_NORESTART; + unsigned long flags; - if (!atomic_read(&stime->running)) - return HRTIMER_NORESTART; + spin_lock_irqsave(&stime->lock, flags); + if (!stime->running) + goto unlock; oruns = hrtimer_forward_now(hrt, ns_to_ktime(t->sticks * resolution)); - snd_timer_interrupt(stime->timer, t->sticks * oruns); + irq_ret = snd_timer_interrupt(stime->timer, t->sticks * oruns); + + switch (irq_ret) { + case SNDRV_TIMER_RET_START: + hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution), + HRTIMER_MODE_REL); + /* fallthru */ + case SNDRV_TIMER_RET_NONE: + ret = HRTIMER_RESTART; + break; + default: + stime->running = false; + break; /* HRTIMER_NORESTART */ + } - if (!atomic_read(&stime->running)) - return HRTIMER_NORESTART; - return HRTIMER_RESTART; + unlock: + spin_unlock_irqrestore(&stime->lock, flags); + return ret; } static int snd_hrtimer_open(struct snd_timer *t) @@ -68,7 +86,8 @@ static int snd_hrtimer_open(struct snd_timer *t) hrtimer_init(&stime->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL); stime->timer = t; stime->hrt.function = snd_hrtimer_callback; - atomic_set(&stime->running, 0); + spin_lock_init(&stime->lock); + stime->running = false; t->private_data = stime; return 0; } @@ -88,25 +107,31 @@ static int snd_hrtimer_close(struct snd_timer *t) static int snd_hrtimer_start(struct snd_timer *t) { struct snd_hrtimer *stime = t->private_data; + unsigned long flags; - atomic_set(&stime->running, 0); - hrtimer_try_to_cancel(&stime->hrt); + spin_lock_irqsave(&stime->lock, flags); hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution), HRTIMER_MODE_REL); - atomic_set(&stime->running, 1); + stime->running = true; + spin_unlock_irqrestore(&stime->lock, flags); return 0; } static int snd_hrtimer_stop(struct snd_timer *t) { struct snd_hrtimer *stime = t->private_data; - atomic_set(&stime->running, 0); + unsigned long flags; + + spin_lock_irqsave(&stime->lock, flags); hrtimer_try_to_cancel(&stime->hrt); + stime->running = false; + spin_unlock_irqrestore(&stime->lock, flags); return 0; } static struct snd_timer_hardware hrtimer_hw = { - .flags = SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET, + .flags = SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET | + SNDRV_TIMER_HW_RET_CTRL, .open = snd_hrtimer_open, .close = snd_hrtimer_close, .start = snd_hrtimer_start, -- 2.8.1 --Multipart_Wed_Apr_20_12:31:00_2016-1--