On Tue, 29 Mar 2022 23:32:25 +0200, syzbot wrote:
Hello,
syzbot found the following issue on:
HEAD commit: 8515d05bf6bc Add linux-next specific files for 20220328 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=15c0acc7700000 kernel config: https://syzkaller.appspot.com/x/.config?x=530c68bef4e2b8a8 dashboard link: https://syzkaller.appspot.com/bug?extid=6e5c88838328e99c7e1c compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14433ca5700000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=163bb77d700000
IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+6e5c88838328e99c7e1c@syzkaller.appspotmail.com
====================================================== WARNING: possible circular locking dependency detected 5.17.0-next-20220328-syzkaller #0 Not tainted
syz-executor329/3588 is trying to acquire lock: ffff8880243c1d28 (&mm->mmap_lock#2){++++}-{3:3}, at: __might_fault+0xa1/0x170 mm/memory.c:5300
but task is already holding lock: ffff88801afef230 (&runtime->buffer_mutex){+.+.}-{3:3}, at: wait_for_avail sound/core/pcm_lib.c:1913 [inline] ffff88801afef230 (&runtime->buffer_mutex){+.+.}-{3:3}, at: __snd_pcm_lib_xfer+0xbca/0x1e20 sound/core/pcm_lib.c:2263
OK, that's a similar issue as we've hit in the past for OSS. Now it appears as we're taking a big lock at the whole read/write operations.
The fix patch below.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: pcm: Fix potential AB/BA lock with buffer_mutex and mmap_lock
syzbot caught a potential deadlock between the PCM runtime->buffer_mutex and the mm->mmap_lock. It was brought by the recent fix to cover the racy read/write and other ioctls, and in that commit, I overlooked a (hopefully only) corner case that may take the revert lock, namely, the OSS mmap. The OSS mmap operation exceptionally allows to re-configure the parameters inside the OSS syscall, where mm->mmap_mutex is already held. Meanwhile, the copy_from/to_user calls at read/write operation also takes the mm->mmap_lock internally, hence it may read to a AB/BA deadlock.
A similar problem was seen in the past and we fixed it with a refcount (in commit b248371628aa) -- although this covered only the call paths with OSS read/write and OSS ioctls. Now we need to cover the concurrent access via both ALSA and OSS APIs.
This patch addresses the problem above, by replacing the lock in the read/write operations with a refcount similar as we've used for OSS. The new field, runtime->buffer_accessing, keeps the concurrent read/write operations. Unlike the former buffer_mutex protection, this protects only around the copy_from/to_user() calls; the other codes are basically protected by the PCM stream lock. When it's already blocked (a negative value), it aborts. In the ioctl side, they check the buffer_accessing, and set to a negative value unless it's already being accessed.
Reported-by: syzbot+6e5c88838328e99c7e1c@syzkaller.appspotmail.com Fixes: dca947d4d26d ("ALSA: pcm: Fix races among concurrent read/write and buffer changes") Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 1 + sound/core/pcm.c | 1 + sound/core/pcm_lib.c | 9 +++++---- sound/core/pcm_native.c | 39 ++++++++++++++++++++++++++++++++------- 4 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 314f2779cab5..6b99310b5b88 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -402,6 +402,7 @@ struct snd_pcm_runtime { struct fasync_struct *fasync; bool stop_operating; /* sync_stop will be called */ struct mutex buffer_mutex; /* protect for buffer changes */ + atomic_t buffer_accessing; /* >0: in r/w operation, <0: blocked */
/* -- private section -- */ void *private_data; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index edd9849210f2..977d54320a5c 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -970,6 +970,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
runtime->status->state = SNDRV_PCM_STATE_OPEN; mutex_init(&runtime->buffer_mutex); + atomic_set(&runtime->buffer_accessing, 0);
substream->runtime = runtime; substream->private_data = pcm->private_data; diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index a40a35e51fad..1fc7c50ffa62 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1906,11 +1906,9 @@ static int wait_for_avail(struct snd_pcm_substream *substream, if (avail >= runtime->twake) break; snd_pcm_stream_unlock_irq(substream); - mutex_unlock(&runtime->buffer_mutex);
tout = schedule_timeout(wait_time);
- mutex_lock(&runtime->buffer_mutex); snd_pcm_stream_lock_irq(substream); set_current_state(TASK_INTERRUPTIBLE); switch (runtime->status->state) { @@ -2221,7 +2219,6 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream,
nonblock = !!(substream->f_flags & O_NONBLOCK);
- mutex_lock(&runtime->buffer_mutex); snd_pcm_stream_lock_irq(substream); err = pcm_accessible_state(runtime); if (err < 0) @@ -2276,6 +2273,10 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, err = -EINVAL; goto _end_unlock; } + if (!atomic_inc_unless_negative(&runtime->buffer_accessing)) { + err = -EBUSY; + goto _end_unlock; + } snd_pcm_stream_unlock_irq(substream); if (!is_playback) snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_CPU); @@ -2284,6 +2285,7 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, if (is_playback) snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE); snd_pcm_stream_lock_irq(substream); + atomic_dec(&runtime->buffer_accessing); if (err < 0) goto _end_unlock; err = pcm_accessible_state(runtime); @@ -2313,7 +2315,6 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, if (xfer > 0 && err >= 0) snd_pcm_update_state(substream, runtime); snd_pcm_stream_unlock_irq(substream); - mutex_unlock(&runtime->buffer_mutex); return xfer > 0 ? (snd_pcm_sframes_t)xfer : err; } EXPORT_SYMBOL(__snd_pcm_lib_xfer); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 704fdc9ebf91..2b630b2b64be 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -685,6 +685,24 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, return 0; }
+/* acquire buffer_mutex; if it's in r/w operation, return -EBUSY, otherwise + * block the further r/w operations + */ +static int snd_pcm_buffer_access_lock(struct snd_pcm_runtime *runtime) +{ + if (!atomic_dec_unless_positive(&runtime->buffer_accessing)) + return -EBUSY; + mutex_lock(&runtime->buffer_mutex); + return 0; /* keep buffer_mutex */ +} + +/* clear r/w access flag and release the buffer_mutex */ +static void snd_pcm_buffer_access_unlock(struct snd_pcm_runtime *runtime) +{ + atomic_inc(&runtime->buffer_accessing); + mutex_unlock(&runtime->buffer_mutex); +} + #if IS_ENABLED(CONFIG_SND_PCM_OSS) #define is_oss_stream(substream) ((substream)->oss.oss) #else @@ -695,14 +713,16 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_pcm_runtime *runtime; - int err = 0, usecs; + int err, usecs; unsigned int bits; snd_pcm_uframes_t frames;
if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; runtime = substream->runtime; - mutex_lock(&runtime->buffer_mutex); + err = snd_pcm_buffer_access_lock(runtime); + if (err < 0) + return err; snd_pcm_stream_lock_irq(substream); switch (runtime->status->state) { case SNDRV_PCM_STATE_OPEN: @@ -820,7 +840,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, snd_pcm_lib_free_pages(substream); } unlock: - mutex_unlock(&runtime->buffer_mutex); + snd_pcm_buffer_access_unlock(runtime); return err; }
@@ -865,7 +885,9 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; runtime = substream->runtime; - mutex_lock(&runtime->buffer_mutex); + result = snd_pcm_buffer_access_lock(runtime); + if (result < 0) + return result; snd_pcm_stream_lock_irq(substream); switch (runtime->status->state) { case SNDRV_PCM_STATE_SETUP: @@ -884,7 +906,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); cpu_latency_qos_remove_request(&substream->latency_pm_qos_req); unlock: - mutex_unlock(&runtime->buffer_mutex); + snd_pcm_buffer_access_unlock(runtime); return result; }
@@ -1369,12 +1391,15 @@ static int snd_pcm_action_nonatomic(const struct action_ops *ops,
/* Guarantee the group members won't change during non-atomic action */ down_read(&snd_pcm_link_rwsem); - mutex_lock(&substream->runtime->buffer_mutex); + res = snd_pcm_buffer_access_lock(substream->runtime); + if (res < 0) + goto unlock; if (snd_pcm_stream_linked(substream)) res = snd_pcm_action_group(ops, substream, state, false); else res = snd_pcm_action_single(ops, substream, state); - mutex_unlock(&substream->runtime->buffer_mutex); + snd_pcm_buffer_access_unlock(substream->runtime); + unlock: up_read(&snd_pcm_link_rwsem); return res; }