1. Patchew
  2. linux
  3. View series

[PATCH] ALSA: pcm: oss: annotate data-races around runtime->state

Cen Zhang posted 1 patch 3 weeks, 1 day ago
There is a newer version of this series
include/sound/pcm.h      |  2 +-
sound/core/oss/pcm_oss.c | 34 +++++++++++++++++-----------------
2 files changed, 18 insertions(+), 18 deletions(-)
[PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
Posted by Cen Zhang 3 weeks, 1 day ago 
__snd_pcm_set_state() writes runtime->state under the PCM stream lock:

    runtime->state = state;

However, the OSS I/O functions snd_pcm_oss_write3(), snd_pcm_oss_read3(),
snd_pcm_oss_writev3() and snd_pcm_oss_readv3() read runtime->state
without holding the stream lock, only holding oss.params_lock (a
different mutex that does not synchronize with the stream lock):

    if (runtime->state == SNDRV_PCM_STATE_XRUN || ...)

Since __snd_pcm_set_state() is called from IRQ context (e.g.,
snd_pcm_period_elapsed -> snd_pcm_update_state -> __snd_pcm_xrun ->
snd_pcm_stop -> snd_pcm_post_stop) while the OSS read/write paths
run in process context, these are concurrent accesses that constitute
a data race.

The code handles stale reads gracefully through its retry loop
(re-checking after __snd_pcm_lib_xfer returns -EPIPE), so the race
is not harmful under simple interleaving. However, plain C accesses
are formally undefined under LKMM, and without READ_ONCE the compiler
is free to fuse or cache the loads across loop iterations.

Add WRITE_ONCE() in __snd_pcm_set_state() for the write side and
READ_ONCE() on all lockless reads of runtime->state in the four OSS
I/O functions.

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 include/sound/pcm.h      |  2 +-
 sound/core/oss/pcm_oss.c | 34 +++++++++++++++++-----------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index a7860c047503..a91061ace828 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -725,7 +725,7 @@ static inline int snd_pcm_running(struct snd_pcm_substream *substream)
 static inline void __snd_pcm_set_state(struct snd_pcm_runtime *runtime,
 				       snd_pcm_state_t state)
 {
-	runtime->state = state;
+	WRITE_ONCE(runtime->state, state);
 	runtime->status->state = state; /* copy for mmap */
 }
 
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index d4fd4dfc7fc3..b9277f54fa27 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -1229,12 +1229,12 @@ snd_pcm_sframes_t snd_pcm_oss_write3(struct snd_pcm_substream *substream, const
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int ret;
 	while (1) {
-		if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-		    runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+		if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ||
+		    READ_ONCE(runtime->state) == SNDRV_PCM_STATE_SUSPENDED) {
 #ifdef OSS_DEBUG
 			pcm_dbg(substream->pcm,
 				"pcm_oss: write: recovering from %s\n",
-				runtime->state == SNDRV_PCM_STATE_XRUN ?
+				READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ?
 				"XRUN" : "SUSPEND");
 #endif
 			ret = snd_pcm_oss_prepare(substream);
@@ -1249,7 +1249,7 @@ snd_pcm_sframes_t snd_pcm_oss_write3(struct snd_pcm_substream *substream, const
 			break;
 		/* test, if we can't store new data, because the stream */
 		/* has not been started */
-		if (runtime->state == SNDRV_PCM_STATE_PREPARED)
+		if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_PREPARED)
 			return -EAGAIN;
 	}
 	return ret;
@@ -1261,18 +1261,18 @@ snd_pcm_sframes_t snd_pcm_oss_read3(struct snd_pcm_substream *substream, char *p
 	snd_pcm_sframes_t delay;
 	int ret;
 	while (1) {
-		if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-		    runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+		if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ||
+		    READ_ONCE(runtime->state) == SNDRV_PCM_STATE_SUSPENDED) {
 #ifdef OSS_DEBUG
 			pcm_dbg(substream->pcm,
 				"pcm_oss: read: recovering from %s\n",
-				runtime->state == SNDRV_PCM_STATE_XRUN ?
+				READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ?
 				"XRUN" : "SUSPEND");
 #endif
 			ret = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DRAIN, NULL);
 			if (ret < 0)
 				break;
-		} else if (runtime->state == SNDRV_PCM_STATE_SETUP) {
+		} else if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_SETUP) {
 			ret = snd_pcm_oss_prepare(substream);
 			if (ret < 0)
 				break;
@@ -1285,7 +1285,7 @@ snd_pcm_sframes_t snd_pcm_oss_read3(struct snd_pcm_substream *substream, char *p
 					 frames, in_kernel);
 		mutex_lock(&runtime->oss.params_lock);
 		if (ret == -EPIPE) {
-			if (runtime->state == SNDRV_PCM_STATE_DRAINING) {
+			if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_DRAINING) {
 				ret = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
 				if (ret < 0)
 					break;
@@ -1304,12 +1304,12 @@ snd_pcm_sframes_t snd_pcm_oss_writev3(struct snd_pcm_substream *substream, void
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int ret;
 	while (1) {
-		if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-		    runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+		if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ||
+		    READ_ONCE(runtime->state) == SNDRV_PCM_STATE_SUSPENDED) {
 #ifdef OSS_DEBUG
 			pcm_dbg(substream->pcm,
 				"pcm_oss: writev: recovering from %s\n",
-				runtime->state == SNDRV_PCM_STATE_XRUN ?
+				READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ?
 				"XRUN" : "SUSPEND");
 #endif
 			ret = snd_pcm_oss_prepare(substream);
@@ -1322,7 +1322,7 @@ snd_pcm_sframes_t snd_pcm_oss_writev3(struct snd_pcm_substream *substream, void
 
 		/* test, if we can't store new data, because the stream */
 		/* has not been started */
-		if (runtime->state == SNDRV_PCM_STATE_PREPARED)
+		if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_PREPARED)
 			return -EAGAIN;
 	}
 	return ret;
@@ -1333,18 +1333,18 @@ snd_pcm_sframes_t snd_pcm_oss_readv3(struct snd_pcm_substream *substream, void *
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int ret;
 	while (1) {
-		if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-		    runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+		if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ||
+		    READ_ONCE(runtime->state) == SNDRV_PCM_STATE_SUSPENDED) {
 #ifdef OSS_DEBUG
 			pcm_dbg(substream->pcm,
 				"pcm_oss: readv: recovering from %s\n",
-				runtime->state == SNDRV_PCM_STATE_XRUN ?
+				READ_ONCE(runtime->state) == SNDRV_PCM_STATE_XRUN ?
 				"XRUN" : "SUSPEND");
 #endif
 			ret = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DRAIN, NULL);
 			if (ret < 0)
 				break;
-		} else if (runtime->state == SNDRV_PCM_STATE_SETUP) {
+		} else if (READ_ONCE(runtime->state) == SNDRV_PCM_STATE_SETUP) {
 			ret = snd_pcm_oss_prepare(substream);
 			if (ret < 0)
 				break;
-- 
2.34.1
Re: [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
Posted by Takashi Iwai 3 weeks ago 
On Mon, 16 Mar 2026 04:05:50 +0100,
Cen Zhang wrote:
> 
> __snd_pcm_set_state() writes runtime->state under the PCM stream lock:
> 
>     runtime->state = state;
> 
> However, the OSS I/O functions snd_pcm_oss_write3(), snd_pcm_oss_read3(),
> snd_pcm_oss_writev3() and snd_pcm_oss_readv3() read runtime->state
> without holding the stream lock, only holding oss.params_lock (a
> different mutex that does not synchronize with the stream lock):
> 
>     if (runtime->state == SNDRV_PCM_STATE_XRUN || ...)
> 
> Since __snd_pcm_set_state() is called from IRQ context (e.g.,
> snd_pcm_period_elapsed -> snd_pcm_update_state -> __snd_pcm_xrun ->
> snd_pcm_stop -> snd_pcm_post_stop) while the OSS read/write paths
> run in process context, these are concurrent accesses that constitute
> a data race.
> 
> The code handles stale reads gracefully through its retry loop
> (re-checking after __snd_pcm_lib_xfer returns -EPIPE), so the race
> is not harmful under simple interleaving. However, plain C accesses
> are formally undefined under LKMM, and without READ_ONCE the compiler
> is free to fuse or cache the loads across loop iterations.
> 
> Add WRITE_ONCE() in __snd_pcm_set_state() for the write side and
> READ_ONCE() on all lockless reads of runtime->state in the four OSS
> I/O functions.
> 
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

Thanks for the patch.

I believe it's better not to go with barriers but rather taking the
proper spinlock, as it's only for this OSS layer, and other places are
already doing so.

That said,

- Export snd_pcm_set_state() in pcm_native.c
- Introduce snd_pcm_get_state() helper just to call like

snd_pcm_state_t snd_pcm_get_state(struct snd_pcm_substream *substream)
{
	guard(pcm_stream_lock_irqsave)(substream);
	return substream->runtime->state;
}

- Use those for setting the state in pcm_oss.c appropriately;
  some places are already in the lock, and they don't use the above.
  Also avoid calling snd_pcm_get_state() repeatedly if not needed.

Care to revise the patch and resubmit?


thanks,

Takashi
Re: [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
Posted by Cen Zhang 3 weeks ago 
> I believe it's better not to go with barriers but rather taking the
> proper spinlock, as it's only for this OSS layer, and other places are
> already doing so.
>
> That said,
>
> - Export snd_pcm_set_state() in pcm_native.c
> - Introduce snd_pcm_get_state() helper just to call like
> ...
> Care to revise the patch and resubmit?

Thank you for the review. That makes sense — using the proper stream
lock is indeed cleaner than barriers for this layer.

I've sent the v2 following your suggestions. Please take a look.

Best regards,
Cen Zhang

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.