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

[Takashi Iwai]

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@xxxxxxxxx>

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