From: Arjan van de Ven <arjan@infradead.org> To: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, perex@perex.cz, tiwai@suse.de Subject: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop Date: Mon, 5 Sep 2011 09:49:47 -0700 [thread overview] Message-ID: <20110905094947.6ece87c2@infradead.org> (raw) >From 2e37f0a4b2289962e1a45d8e02f8a7f7adad619f Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <arjan@linux.intel.com> Date: Mon, 5 Sep 2011 09:40:18 -0700 Subject: [PATCH] sound: Fix race condition in the pcm_lib "wait for space" loop The wait_for_avail() function in pcm_lib.c has a race in it (observed in practice by an Intel validation group). The function is supposed to return once space in the buffer has become available, or if some timeout happens. The entity that creates space (irq handler of sound driver and some such) will do a wake up on a waitqueue that this function registers for. However there are two races in the existing code 1) If space became available between the caller noticing there was no space and this function actually sleeping, the wakeup is missed and the timeout condition will happen instead 2) If a wakeup happened but not sufficient space became available, the code will loop again and wait for more space. However, if the second wake comes in prior to hitting the schedule_timeout_interruptible(), it will be missed, and potentially you'll wait out until the timeout happens. The fix consists of using more careful setting of the current state (so that if a wakeup happens in the main loop window, the schedule_timeout() falls through) and by checking for available space prior to going into the schedule_timeout() loop, but after being on the waitqueue and having the state set to interruptible. Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> CC: Jaroslav Kysela <perex@perex.cz> CC: Takashi Iwai <tiwai@suse.de> CC: alsa-devel@alsa-project.org CC: linux-kernel@vger.kernel.org --- sound/core/pcm_lib.c | 29 ++++++++++++++++++++++++++--- 1 files changed, 26 insertions(+), 3 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 86d0caf..8848080 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1761,6 +1761,10 @@ static int wait_for_avail(struct snd_pcm_substream *substream, snd_pcm_uframes_t avail = 0; long wait_time, tout; + init_waitqueue_entry(&wait, current); + add_wait_queue(&runtime->tsleep, &wait); + set_current_state(TASK_INTERRUPTIBLE); + if (runtime->no_period_wakeup) wait_time = MAX_SCHEDULE_TIMEOUT; else { @@ -1771,16 +1775,34 @@ static int wait_for_avail(struct snd_pcm_substream *substream, } wait_time = msecs_to_jiffies(wait_time * 1000); } - init_waitqueue_entry(&wait, current); - add_wait_queue(&runtime->tsleep, &wait); + + /* + * We need to check if space became available already (and thus the + * wakeup happened already) prior to going into the sleep loop to + * close the race of space already having become available. + * This check must happen after been added to the waitqueue and + * having current state be INTERRUPTIBLE. + */ + + if (is_playback) + avail = snd_pcm_playback_avail(runtime); + else + avail = snd_pcm_capture_avail(runtime); + if (avail >= runtime->twake) + goto _endloop; + + for (;;) { if (signal_pending(current)) { err = -ERESTARTSYS; break; } snd_pcm_stream_unlock_irq(substream); - tout = schedule_timeout_interruptible(wait_time); + + tout = schedule_timeout(wait_time); + snd_pcm_stream_lock_irq(substream); + set_current_state(TASK_INTERRUPTIBLE); switch (runtime->status->state) { case SNDRV_PCM_STATE_SUSPENDED: err = -ESTRPIPE; @@ -1814,6 +1836,7 @@ static int wait_for_avail(struct snd_pcm_substream *substream, break; } _endloop: + set_current_state(TASK_RUNNING); remove_wait_queue(&runtime->tsleep, &wait); *availp = avail; return err; -- 1.7.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org
WARNING: multiple messages have this Message-ID (diff)
From: Arjan van de Ven <arjan@infradead.org> To: alsa-devel@alsa-project.org Cc: tiwai@suse.de, akpm@linux-foundation.org, linux-kernel@vger.kernel.org Subject: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop Date: Mon, 5 Sep 2011 09:49:47 -0700 [thread overview] Message-ID: <20110905094947.6ece87c2@infradead.org> (raw) >From 2e37f0a4b2289962e1a45d8e02f8a7f7adad619f Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <arjan@linux.intel.com> Date: Mon, 5 Sep 2011 09:40:18 -0700 Subject: [PATCH] sound: Fix race condition in the pcm_lib "wait for space" loop The wait_for_avail() function in pcm_lib.c has a race in it (observed in practice by an Intel validation group). The function is supposed to return once space in the buffer has become available, or if some timeout happens. The entity that creates space (irq handler of sound driver and some such) will do a wake up on a waitqueue that this function registers for. However there are two races in the existing code 1) If space became available between the caller noticing there was no space and this function actually sleeping, the wakeup is missed and the timeout condition will happen instead 2) If a wakeup happened but not sufficient space became available, the code will loop again and wait for more space. However, if the second wake comes in prior to hitting the schedule_timeout_interruptible(), it will be missed, and potentially you'll wait out until the timeout happens. The fix consists of using more careful setting of the current state (so that if a wakeup happens in the main loop window, the schedule_timeout() falls through) and by checking for available space prior to going into the schedule_timeout() loop, but after being on the waitqueue and having the state set to interruptible. Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> CC: Jaroslav Kysela <perex@perex.cz> CC: Takashi Iwai <tiwai@suse.de> CC: alsa-devel@alsa-project.org CC: linux-kernel@vger.kernel.org --- sound/core/pcm_lib.c | 29 ++++++++++++++++++++++++++--- 1 files changed, 26 insertions(+), 3 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 86d0caf..8848080 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1761,6 +1761,10 @@ static int wait_for_avail(struct snd_pcm_substream *substream, snd_pcm_uframes_t avail = 0; long wait_time, tout; + init_waitqueue_entry(&wait, current); + add_wait_queue(&runtime->tsleep, &wait); + set_current_state(TASK_INTERRUPTIBLE); + if (runtime->no_period_wakeup) wait_time = MAX_SCHEDULE_TIMEOUT; else { @@ -1771,16 +1775,34 @@ static int wait_for_avail(struct snd_pcm_substream *substream, } wait_time = msecs_to_jiffies(wait_time * 1000); } - init_waitqueue_entry(&wait, current); - add_wait_queue(&runtime->tsleep, &wait); + + /* + * We need to check if space became available already (and thus the + * wakeup happened already) prior to going into the sleep loop to + * close the race of space already having become available. + * This check must happen after been added to the waitqueue and + * having current state be INTERRUPTIBLE. + */ + + if (is_playback) + avail = snd_pcm_playback_avail(runtime); + else + avail = snd_pcm_capture_avail(runtime); + if (avail >= runtime->twake) + goto _endloop; + + for (;;) { if (signal_pending(current)) { err = -ERESTARTSYS; break; } snd_pcm_stream_unlock_irq(substream); - tout = schedule_timeout_interruptible(wait_time); + + tout = schedule_timeout(wait_time); + snd_pcm_stream_lock_irq(substream); + set_current_state(TASK_INTERRUPTIBLE); switch (runtime->status->state) { case SNDRV_PCM_STATE_SUSPENDED: err = -ESTRPIPE; @@ -1814,6 +1836,7 @@ static int wait_for_avail(struct snd_pcm_substream *substream, break; } _endloop: + set_current_state(TASK_RUNNING); remove_wait_queue(&runtime->tsleep, &wait); *availp = avail; return err; -- 1.7.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org
next reply other threads:[~2011-09-05 16:46 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-09-05 16:49 Arjan van de Ven [this message] 2011-09-05 16:49 ` [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop Arjan van de Ven 2011-09-06 22:59 ` Andrew Morton 2011-09-06 22:59 ` Andrew Morton 2011-09-07 3:07 ` Arjan van de Ven 2011-09-07 3:07 ` Arjan van de Ven 2011-09-12 9:57 ` [alsa-devel] " Takashi Iwai 2011-09-15 7:11 ` Takashi Iwai 2011-09-15 7:11 ` Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110905094947.6ece87c2@infradead.org \ --to=arjan@infradead.org \ --cc=akpm@linux-foundation.org \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.