[alsa-devel] [PATCH 0/2] Yet more hardening of ALSA hrtimer races
Hi,
this is a resubmission of patches for hardening the ALSA hrtimer backend against open/close races.
Takashi
===
Takashi Iwai (2): ALSA: timer: Allow backend disabling start/stop from handler ALSA: hrtimer: Use manual start/stop in callback
include/sound/timer.h | 12 +++++++++++- sound/core/hrtimer.c | 51 ++++++++++++++++++++++++++++++++++++++------------- sound/core/timer.c | 24 ++++++++++++++++++------ 3 files changed, 67 insertions(+), 20 deletions(-)
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; }
/*
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,
participants (1)
-
Takashi Iwai