Re: [syzbot] [sound?] possible deadlock in __snd_pcm_lib_xfer (2)

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-08-29 17:06:02 [-0700], syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit d2d6422f8bd17c6bb205133e290625a564194496
> Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date:   Fri Sep 6 10:59:04 2024 +0000
> 
>     x86: Allow to enable PREEMPT_RT.
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=12db5634580000
> start commit:   07d9df80082b Merge tag 'perf-tools-fixes-for-v6.17-2025-08..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=11db5634580000
> console output: https://syzkaller.appspot.com/x/log.txt?x=16db5634580000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=e1e1566c7726877e
> dashboard link: https://syzkaller.appspot.com/bug?extid=10b4363fb0f46527f3f3
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10307262580000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17110242580000
> 
> Reported-by: syzbot+10b4363fb0f46527f3f3@syzkaller.appspotmail.com
> Fixes: d2d6422f8bd1 ("x86: Allow to enable PREEMPT_RT.")

This is unfortunate. There is nothing that sound did wrong, it is rather
special softirq handling in this case. We don't see this often because
it requires that a timer is cancelled at the time it is running.
The assumption made by sound is that spin_lock_irq() also disables
softirqs. This is not the case on PREEMPT_RT.

The hunk below avoids the splat. Adding local_bh_disable() to
spin_lock_irq() would cure it, too. It would also result in random
synchronisation points across the kernel leading to something less
usable.
The imho best solution would to get rid of softirq_ctrl.lock which has
been proposed
	https://lore.kernel.org/all/20250901163811.963326-4-bigeasy@linutronix.de/

Comments?

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -84,19 +84,24 @@ void snd_pcm_group_init(struct snd_pcm_group *group)
 }
 
 /* define group lock helpers */
-#define DEFINE_PCM_GROUP_LOCK(action, mutex_action) \
+#define DEFINE_PCM_GROUP_LOCK(action, bh_lock, bh_unlock, mutex_action) \
 static void snd_pcm_group_ ## action(struct snd_pcm_group *group, bool nonatomic) \
 { \
-	if (nonatomic) \
+	if (nonatomic) { \
 		mutex_ ## mutex_action(&group->mutex); \
-	else \
-		spin_ ## action(&group->lock); \
+	} else { \
+		if (IS_ENABLED(CONFIG_PREEMPT_RT) && bh_lock)	\
+			local_bh_disable();			\
+		spin_ ## action(&group->lock);			\
+		if (IS_ENABLED(CONFIG_PREEMPT_RT) && bh_unlock)	\
+			local_bh_enable();			\
+	}							\
 }
 
-DEFINE_PCM_GROUP_LOCK(lock, lock);
-DEFINE_PCM_GROUP_LOCK(unlock, unlock);
-DEFINE_PCM_GROUP_LOCK(lock_irq, lock);
-DEFINE_PCM_GROUP_LOCK(unlock_irq, unlock);
+DEFINE_PCM_GROUP_LOCK(lock, 0, 0, lock);
+DEFINE_PCM_GROUP_LOCK(unlock, 0, 0, unlock);
+DEFINE_PCM_GROUP_LOCK(lock_irq, 1, 0, lock);
+DEFINE_PCM_GROUP_LOCK(unlock_irq, 0, 1, unlock);
 
 /**
  * snd_pcm_stream_lock - Lock the PCM stream


Sebastian