* [PATCH 1/2] ALSA: timer: Allow backend disabling start/stop from handler
2016-04-21 14:13 [PATCH 0/2] Yet more hardening of ALSA hrtimer races Takashi Iwai
@ 2016-04-21 14:13 ` Takashi Iwai
2016-04-21 14:13 ` [PATCH 2/2] ALSA: hrtimer: Use manual start/stop in callback Takashi Iwai
1 sibling, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2016-04-21 14:13 UTC (permalink / raw)
To: alsa-devel
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.
The actual usage of this flag will be done in the later patch,
starting from hrtimer.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] ALSA: hrtimer: Use manual start/stop in callback
2016-04-21 14:13 [PATCH 0/2] Yet more hardening of ALSA hrtimer races Takashi Iwai
2016-04-21 14:13 ` [PATCH 1/2] ALSA: timer: Allow backend disabling start/stop from handler Takashi Iwai
@ 2016-04-21 14:13 ` Takashi Iwai
1 sibling, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2016-04-21 14:13 UTC (permalink / raw)
To: alsa-devel
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.
Hopefully this fixes the still remaining races spotted (but fairly
rarely) by syzkaller fuzzer.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread