[PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Arjan van de Ven]

>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

[PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Arjan van de Ven]

>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

Re: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Andrew Morton]

On Mon, 5 Sep 2011 09:49:47 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> >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.
> 
> ...
>
> --- 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);

Well, this isn't very good either.  if a wakeup gets delivered to
runtime->tsleep before the set_current_state(), this process will go
ahead and incorrectly set itself into TASK_INTERRUPTIBLE state.

That looks like it will be dont-care/cant-happen in this case, but it's
setting a bad example.

Re: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Andrew Morton]

On Mon, 5 Sep 2011 09:49:47 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> >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.
> 
> ...
>
> --- 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);

Well, this isn't very good either.  if a wakeup gets delivered to
runtime->tsleep before the set_current_state(), this process will go
ahead and incorrectly set itself into TASK_INTERRUPTIBLE state.

That looks like it will be dont-care/cant-happen in this case, but it's
setting a bad example.

Re: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Arjan van de Ven]

On Tue, 6 Sep 2011 15:59:54 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> > --- 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);
> 
> Well, this isn't very good either.  if a wakeup gets delivered to
> runtime->tsleep before the set_current_state(), this process will go
> ahead and incorrectly set itself into TASK_INTERRUPTIBLE state.

... and then check the condition for exit, and then set it to
TASK_RUNABLE a the end...

> 
> That looks like it will be dont-care/cant-happen in this case, but
> it's setting a bad example.

agreed that your ordering is more clean/better.....



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Arjan van de Ven]

On Tue, 6 Sep 2011 15:59:54 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> > --- 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);
> 
> Well, this isn't very good either.  if a wakeup gets delivered to
> runtime->tsleep before the set_current_state(), this process will go
> ahead and incorrectly set itself into TASK_INTERRUPTIBLE state.

... and then check the condition for exit, and then set it to
TASK_RUNABLE a the end...

> 
> That looks like it will be dont-care/cant-happen in this case, but
> it's setting a bad example.

agreed that your ordering is more clean/better.....



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [alsa-devel] [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Takashi Iwai]

Hi,

sorry for the late follow-up, as I've been on vacation until today.

At Mon, 5 Sep 2011 09:49:47 -0700,
Arjan van de Ven wrote:
> 
> >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;
> +

Instead of adding this, we can move the check in the for loop at the
beginning of the loop, no?


>  	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;


thanks,

Takashi

Re: [alsa-devel] [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Takashi Iwai]

At Mon, 12 Sep 2011 11:57:31 +0200,
Takashi Iwai wrote:
> 
> Hi,
> 
> sorry for the late follow-up, as I've been on vacation until today.
> 
> At Mon, 5 Sep 2011 09:49:47 -0700,
> Arjan van de Ven wrote:
> > 
> > >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;
> > +
> 
> Instead of adding this, we can move the check in the for loop at the
> beginning of the loop, no?

FYI, I applied the patch below to my tree.  It contains Andrew's fix
and my slight refactoring.


thanks,

Takashi

---
From: Arjan van de Ven <arjan@infradead.org>
Subject: ALSA: pcm - fix race condition in wait_for_avail()

wait_for_avail() 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.

[tiwai: the following changes have been added to Arjan's original patch:
 - merged akpm's fix for waitqueue adding order into a single patch
 - reduction of duplicated code of avail check
]

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Cc: <stable@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_lib.c |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 86d0caf..62e90b8 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);
+	set_current_state(TASK_INTERRUPTIBLE);
+	add_wait_queue(&runtime->tsleep, &wait);
+
 	if (runtime->no_period_wakeup)
 		wait_time = MAX_SCHEDULE_TIMEOUT;
 	else {
@@ -1771,16 +1775,32 @@ 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);
+
 	for (;;) {
 		if (signal_pending(current)) {
 			err = -ERESTARTSYS;
 			break;
 		}
+
+		/*
+		 * We need to check if space became available already
+		 * (and thus the wakeup happened already) first 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)
+			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;
@@ -1806,14 +1826,9 @@ static int wait_for_avail(struct snd_pcm_substream *substream,
 			err = -EIO;
 			break;
 		}
-		if (is_playback)
-			avail = snd_pcm_playback_avail(runtime);
-		else
-			avail = snd_pcm_capture_avail(runtime);
-		if (avail >= runtime->twake)
-			break;
 	}
  _endloop:
+	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&runtime->tsleep, &wait);
 	*availp = avail;
 	return err;
-- 
1.7.6.1

Re: [PATCH] sound: Fix race condition in the pcm_lib "wait for space loop

[Takashi Iwai]

At Mon, 12 Sep 2011 11:57:31 +0200,
Takashi Iwai wrote:
> 
> Hi,
> 
> sorry for the late follow-up, as I've been on vacation until today.
> 
> At Mon, 5 Sep 2011 09:49:47 -0700,
> Arjan van de Ven wrote:
> > 
> > >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;
> > +
> 
> Instead of adding this, we can move the check in the for loop at the
> beginning of the loop, no?

FYI, I applied the patch below to my tree.  It contains Andrew's fix
and my slight refactoring.


thanks,

Takashi

---
From: Arjan van de Ven <arjan@infradead.org>
Subject: ALSA: pcm - fix race condition in wait_for_avail()

wait_for_avail() 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.

[tiwai: the following changes have been added to Arjan's original patch:
 - merged akpm's fix for waitqueue adding order into a single patch
 - reduction of duplicated code of avail check
]

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Cc: <stable@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_lib.c |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 86d0caf..62e90b8 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);
+	set_current_state(TASK_INTERRUPTIBLE);
+	add_wait_queue(&runtime->tsleep, &wait);
+
 	if (runtime->no_period_wakeup)
 		wait_time = MAX_SCHEDULE_TIMEOUT;
 	else {
@@ -1771,16 +1775,32 @@ 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);
+
 	for (;;) {
 		if (signal_pending(current)) {
 			err = -ERESTARTSYS;
 			break;
 		}
+
+		/*
+		 * We need to check if space became available already
+		 * (and thus the wakeup happened already) first 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)
+			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;
@@ -1806,14 +1826,9 @@ static int wait_for_avail(struct snd_pcm_substream *substream,
 			err = -EIO;
 			break;
 		}
-		if (is_playback)
-			avail = snd_pcm_playback_avail(runtime);
-		else
-			avail = snd_pcm_capture_avail(runtime);
-		if (avail >= runtime->twake)
-			break;
 	}
  _endloop:
+	set_current_state(TASK_RUNNING);
 	remove_wait_queue(&runtime->tsleep, &wait);
 	*availp = avail;
 	return err;
-- 
1.7.6.1