* [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
@ 2026-03-16 3:05 Cen Zhang
2026-03-16 8:40 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: Cen Zhang @ 2026-03-16 3:05 UTC (permalink / raw)
To: perex, tiwai, chleroy
Cc: linux-sound, linux-kernel, baijiaju1990, r33s3n6, gality369,
zhenghaoran154, hanguidong02, ziyuzhang201, Cen Zhang
__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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
2026-03-16 3:05 [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state Cen Zhang
@ 2026-03-16 8:40 ` Takashi Iwai
2026-03-16 9:07 ` Cen Zhang
0 siblings, 1 reply; 4+ messages in thread
From: Takashi Iwai @ 2026-03-16 8:40 UTC (permalink / raw)
To: Cen Zhang
Cc: perex, tiwai, chleroy, linux-sound, linux-kernel, baijiaju1990,
r33s3n6, gality369, zhenghaoran154, hanguidong02, ziyuzhang201
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
2026-03-16 8:40 ` Takashi Iwai
@ 2026-03-16 9:07 ` Cen Zhang
0 siblings, 0 replies; 4+ messages in thread
From: Cen Zhang @ 2026-03-16 9:07 UTC (permalink / raw)
To: Takashi Iwai
Cc: perex, tiwai, chleroy, linux-sound, linux-kernel, baijiaju1990,
r33s3n6, gality369, zhenghaoran154, hanguidong02, ziyuzhang201
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state
@ 2026-03-16 6:48 Cen Zhang
0 siblings, 0 replies; 4+ messages in thread
From: Cen Zhang @ 2026-03-16 6:48 UTC (permalink / raw)
To: perex, chleroy
Cc: tiwai, linux-sound, linux-kernel, baijiaju1990, r33s3n6,
gality369, zhenghaoran154, hanguidong02, ziyuzhang201, Cen Zhang
__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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-03-16 9:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-16 3:05 [PATCH] ALSA: pcm: oss: annotate data-races around runtime->state Cen Zhang
2026-03-16 8:40 ` Takashi Iwai
2026-03-16 9:07 ` Cen Zhang
-- strict thread matches above, loose matches on Subject: below --
2026-03-16 6:48 Cen Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox