ALSA: pcm: oss: use proper stream lock for runtime->state access

__snd_pcm_set_state() writes runtime->state under the PCM stream lock.
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).

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.

Rather than using READ_ONCE()/WRITE_ONCE() barriers, introduce a
snd_pcm_get_state() helper that reads runtime->state under the stream
lock, matching the locking discipline used elsewhere in the PCM layer.
Also export snd_pcm_set_state() for completeness.

Use snd_pcm_get_state() in all four OSS I/O functions, caching the
result in a local variable where the same snapshot is used for
multiple comparisons to avoid taking the lock repeatedly.

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Link: https://patch.msgid.link/20260316085047.2876451-1-zzzccc427@gmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index a7860c047503a84ef035d7735ce2a90af7a2e720..76fc33dce537dd3f604619dacc4e79afb93fcb7b 100644 (file)
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -729,6 +729,10 @@ static inline void __snd_pcm_set_state(struct snd_pcm_runtime *runtime,
        runtime->status->state = state; /* copy for mmap */
 }
 
+void snd_pcm_set_state(struct snd_pcm_substream *substream,
+                      snd_pcm_state_t state);
+snd_pcm_state_t snd_pcm_get_state(struct snd_pcm_substream *substream);
+
 /**
  * bytes_to_samples - Unit conversion of the size from bytes to samples
  * @runtime: PCM runtime instance

diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index d4fd4dfc7fc3126d6f09ea0b0b08a7a5bc37bce0..a140a0d9abb808288cb7b773a2f553a71df9f38b 100644 (file)
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -1227,14 +1227,16 @@ static int snd_pcm_oss_capture_position_fixup(struct snd_pcm_substream *substrea
 snd_pcm_sframes_t snd_pcm_oss_write3(struct snd_pcm_substream *substream, const char *ptr, snd_pcm_uframes_t frames, int in_kernel)
 {
        struct snd_pcm_runtime *runtime = substream->runtime;
+       snd_pcm_state_t state;
        int ret;
        while (1) {
-               if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-                   runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+               state = snd_pcm_get_state(substream);
+               if (state == SNDRV_PCM_STATE_XRUN ||
+                   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 ?
+                               state == SNDRV_PCM_STATE_XRUN ?
                                "XRUN" : "SUSPEND");
 #endif
                        ret = snd_pcm_oss_prepare(substream);
@@ -1249,7 +1251,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 (snd_pcm_get_state(substream) == SNDRV_PCM_STATE_PREPARED)
                        return -EAGAIN;
        }
        return ret;
@@ -1259,20 +1261,22 @@ snd_pcm_sframes_t snd_pcm_oss_read3(struct snd_pcm_substream *substream, char *p
 {
        struct snd_pcm_runtime *runtime = substream->runtime;
        snd_pcm_sframes_t delay;
+       snd_pcm_state_t state;
        int ret;
        while (1) {
-               if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-                   runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+               state = snd_pcm_get_state(substream);
+               if (state == SNDRV_PCM_STATE_XRUN ||
+                   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 ?
+                               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 (state == SNDRV_PCM_STATE_SETUP) {
                        ret = snd_pcm_oss_prepare(substream);
                        if (ret < 0)
                                break;
@@ -1285,7 +1289,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 (snd_pcm_get_state(substream) == SNDRV_PCM_STATE_DRAINING) {
                                ret = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
                                if (ret < 0)
                                        break;
@@ -1301,15 +1305,16 @@ snd_pcm_sframes_t snd_pcm_oss_read3(struct snd_pcm_substream *substream, char *p
 #ifdef CONFIG_SND_PCM_OSS_PLUGINS
 snd_pcm_sframes_t snd_pcm_oss_writev3(struct snd_pcm_substream *substream, void **bufs, snd_pcm_uframes_t frames)
 {
-       struct snd_pcm_runtime *runtime = substream->runtime;
+       snd_pcm_state_t state;
        int ret;
        while (1) {
-               if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-                   runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+               state = snd_pcm_get_state(substream);
+               if (state == SNDRV_PCM_STATE_XRUN ||
+                   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 ?
+                               state == SNDRV_PCM_STATE_XRUN ?
                                "XRUN" : "SUSPEND");
 #endif
                        ret = snd_pcm_oss_prepare(substream);
@@ -1322,7 +1327,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 (snd_pcm_get_state(substream) == SNDRV_PCM_STATE_PREPARED)
                        return -EAGAIN;
        }
        return ret;
@@ -1330,21 +1335,22 @@ snd_pcm_sframes_t snd_pcm_oss_writev3(struct snd_pcm_substream *substream, void
        
 snd_pcm_sframes_t snd_pcm_oss_readv3(struct snd_pcm_substream *substream, void **bufs, snd_pcm_uframes_t frames)
 {
-       struct snd_pcm_runtime *runtime = substream->runtime;
+       snd_pcm_state_t state;
        int ret;
        while (1) {
-               if (runtime->state == SNDRV_PCM_STATE_XRUN ||
-                   runtime->state == SNDRV_PCM_STATE_SUSPENDED) {
+               state = snd_pcm_get_state(substream);
+               if (state == SNDRV_PCM_STATE_XRUN ||
+                   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 ?
+                               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 (state == SNDRV_PCM_STATE_SETUP) {
                        ret = snd_pcm_oss_prepare(substream);
                        if (ret < 0)
                                break;

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 67cf6a0e17ba8d636ce5003186e5fd282cec76cb..394f86bc4d2920d4c750249691628464d03e3134 100644 (file)
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -618,13 +618,32 @@ static int period_to_usecs(struct snd_pcm_runtime *runtime)
        return usecs;
 }
 
-static void snd_pcm_set_state(struct snd_pcm_substream *substream,
-                             snd_pcm_state_t state)
+/**
+ * snd_pcm_set_state - Set the PCM runtime state with stream lock
+ * @substream: PCM substream
+ * @state: state to set
+ */
+void snd_pcm_set_state(struct snd_pcm_substream *substream,
+                      snd_pcm_state_t state)
 {
        guard(pcm_stream_lock_irq)(substream);
        if (substream->runtime->state != SNDRV_PCM_STATE_DISCONNECTED)
                __snd_pcm_set_state(substream->runtime, state);
 }
+EXPORT_SYMBOL_GPL(snd_pcm_set_state);
+
+/**
+ * snd_pcm_get_state - Read the PCM runtime state with stream lock
+ * @substream: PCM substream
+ *
+ * Return: the current PCM state
+ */
+snd_pcm_state_t snd_pcm_get_state(struct snd_pcm_substream *substream)
+{
+       guard(pcm_stream_lock_irqsave)(substream);
+       return substream->runtime->state;
+}
+EXPORT_SYMBOL_GPL(snd_pcm_get_state);
 
 static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream,
                                        int event)