git: c9d9975cd4dd - stable/14 - sound: Fix lock order reversals in mseq_open()

From: Christos Margiolis <christos_at_FreeBSD.org>
Date: Wed, 10 Jul 2024 16:49:00 UTC
The branch stable/14 has been updated by christos:

URL: https://cgit.FreeBSD.org/src/commit/?id=c9d9975cd4dd85414c4bc1e5cafb118544d5b4e7

commit c9d9975cd4dd85414c4bc1e5cafb118544d5b4e7
Author:     Christos Margiolis <christos@FreeBSD.org>
AuthorDate: 2024-07-06 18:22:21 +0000
Commit:     Christos Margiolis <christos@FreeBSD.org>
CommitDate: 2024-07-10 16:48:12 +0000

    sound: Fix lock order reversals in mseq_open()
    
    Opening /dev/sequencer after a clean reboot yields:
    
    lock order reversal: (sleepable after non-sleepable)
     1st 0xfffffe004a2c2c08 seqflq (seqflq, sleep mutex) @ /mnt/src/sys/dev/sound/midi/sequencer.c:754
     2nd 0xffffffff84197ed8 midistat lock (midistat lock, sx) @ /mnt/src/sys/dev/sound/midi/midi.c:1478
    lock order seqflq -> midistat lock attempted at:
    0xffffffff811c9029 at witness_checkorder+0x12b9
    0xffffffff810f18a7 at _sx_xlock+0xf7
    0xffffffff8417f992 at midimapper_open+0x22
    0xffffffff84182770 at mseq_open+0xf0
    0xffffffff80e3380f at devfs_open+0x30f
    0xffffffff81b8b4b7 at VOP_OPEN_APV+0x57
    0xffffffff812da1e7 at vn_open_vnode+0x397
    0xffffffff812d96b3 at vn_open_cred+0xb23
    0xffffffff812c2c6b at openatfp+0x52b
    0xffffffff812c2711 at sys_openat+0x81
    0xffffffff84110579 at filemon_wrapper_openat+0x19
    0xffffffff81a223ae at amd64_syscall+0x39e
    0xffffffff819dd0eb at fast_syscall_common+0xf8
    
    Expose midistat_lock to midi/midi.c so that we can acquire the lock from
    mseq_open() before we lock seq_lock, and introduce _locked variants of
    midimapper_open() and midimapper_fetch_synth().
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 days
    Reviewed by:    dev_submerge.ch, emaste
    Differential Revision:  https://reviews.freebsd.org/D45770
    
    (cherry picked from commit fc76e24e583d45a3a59fd7ad4e603c0679eaf572)
---
 sys/dev/sound/midi/midi.c      | 41 ++++++++++++++++++++++++++++++++---------
 sys/dev/sound/midi/midi.h      |  4 ++++
 sys/dev/sound/midi/sequencer.c |  8 ++++++--
 3 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/sys/dev/sound/midi/midi.c b/sys/dev/sound/midi/midi.c
index 8dd4546d58c5..dab8fe559a0d 100644
--- a/sys/dev/sound/midi/midi.c
+++ b/sys/dev/sound/midi/midi.c
@@ -182,7 +182,8 @@ TAILQ_HEAD(, snd_midi) midi_devs;
  * /dev/midistat variables and declarations, protected by midistat_lock
  */
 
-static struct sx midistat_lock;
+struct sx midistat_lock;
+
 static int      midistat_isopen = 0;
 static struct sbuf midistat_sbuf;
 static struct cdev *midistat_dev;
@@ -1471,16 +1472,28 @@ midimapper_addseq(void *arg1, int *unit, void **cookie)
 }
 
 int
-midimapper_open(void *arg1, void **cookie)
+midimapper_open_locked(void *arg1, void **cookie)
 {
 	int retval = 0;
 	struct snd_midi *m;
 
-	sx_xlock(&midistat_lock);
+	sx_assert(&midistat_lock, SX_XLOCKED);
 	TAILQ_FOREACH(m, &midi_devs, link) {
 		retval++;
 	}
+
+	return retval;
+}
+
+int
+midimapper_open(void *arg1, void **cookie)
+{
+	int retval;
+
+	sx_xlock(&midistat_lock);
+	retval = midimapper_open_locked(arg1, cookie);
 	sx_xunlock(&midistat_lock);
+
 	return retval;
 }
 
@@ -1491,22 +1504,32 @@ midimapper_close(void *arg1, void *cookie)
 }
 
 kobj_t
-midimapper_fetch_synth(void *arg, void *cookie, int unit)
+midimapper_fetch_synth_locked(void *arg, void *cookie, int unit)
 {
 	struct snd_midi *m;
 	int retval = 0;
 
-	sx_xlock(&midistat_lock);
+	sx_assert(&midistat_lock, SX_XLOCKED);
 	TAILQ_FOREACH(m, &midi_devs, link) {
-		if (unit == retval) {
-			sx_xunlock(&midistat_lock);
+		if (unit == retval)
 			return (kobj_t)m->synth;
-		}
 		retval++;
 	}
-	sx_xunlock(&midistat_lock);
+
 	return NULL;
 }
 
+kobj_t
+midimapper_fetch_synth(void *arg, void *cookie, int unit)
+{
+	kobj_t synth;
+
+	sx_xlock(&midistat_lock);
+	synth = midimapper_fetch_synth_locked(arg, cookie, unit);
+	sx_xunlock(&midistat_lock);
+
+	return synth;
+}
+
 DEV_MODULE(midi, midi_modevent, NULL);
 MODULE_VERSION(midi, 1);
diff --git a/sys/dev/sound/midi/midi.h b/sys/dev/sound/midi/midi.h
index 567279d1e654..b200eed9bc74 100644
--- a/sys/dev/sound/midi/midi.h
+++ b/sys/dev/sound/midi/midi.h
@@ -41,6 +41,8 @@ MALLOC_DECLARE(M_MIDI);
 
 #define MIDI_TYPE unsigned char
 
+extern struct sx midistat_lock;
+
 struct snd_midi;
 
 struct snd_midi *
@@ -50,8 +52,10 @@ int	midi_out(struct snd_midi *_m, MIDI_TYPE *_buf, int _size);
 int	midi_in(struct snd_midi *_m, MIDI_TYPE *_buf, int _size);
 
 kobj_t	midimapper_addseq(void *arg1, int *unit, void **cookie);
+int	midimapper_open_locked(void *arg1, void **cookie);
 int	midimapper_open(void *arg1, void **cookie);
 int	midimapper_close(void *arg1, void *cookie);
+kobj_t	midimapper_fetch_synth_locked(void *arg, void *cookie, int unit);
 kobj_t	midimapper_fetch_synth(void *arg, void *cookie, int unit);
 
 #endif
diff --git a/sys/dev/sound/midi/sequencer.c b/sys/dev/sound/midi/sequencer.c
index a943e02030aa..3e4c7cf3edf8 100644
--- a/sys/dev/sound/midi/sequencer.c
+++ b/sys/dev/sound/midi/sequencer.c
@@ -65,6 +65,7 @@
 #include <sys/kthread.h>
 #include <sys/unistd.h>
 #include <sys/selinfo.h>
+#include <sys/sx.h>
 
 #ifdef HAVE_KERNEL_OPTION_HEADERS
 #include "opt_snd.h"
@@ -752,6 +753,7 @@ mseq_open(struct cdev *i_dev, int flags, int mode, struct thread *td)
 	 * Mark this device busy.
 	 */
 
+	sx_xlock(&midistat_lock);
 	mtx_lock(&scp->seq_lock);
 	if (scp->busy) {
 		mtx_unlock(&scp->seq_lock);
@@ -769,14 +771,15 @@ mseq_open(struct cdev *i_dev, int flags, int mode, struct thread *td)
 	 * Enumerate the available midi devices
 	 */
 	scp->midi_number = 0;
-	scp->maxunits = midimapper_open(scp->mapper, &scp->mapper_cookie);
+	scp->maxunits = midimapper_open_locked(scp->mapper, &scp->mapper_cookie);
 
 	if (scp->maxunits == 0)
 		SEQ_DEBUG(2, printf("seq_open: no midi devices\n"));
 
 	for (i = 0; i < scp->maxunits; i++) {
 		scp->midis[scp->midi_number] =
-		    midimapper_fetch_synth(scp->mapper, scp->mapper_cookie, i);
+		    midimapper_fetch_synth_locked(scp->mapper,
+		    scp->mapper_cookie, i);
 		if (scp->midis[scp->midi_number]) {
 			if (SYNTH_OPEN(scp->midis[scp->midi_number], scp,
 				scp->fflags) != 0)
@@ -788,6 +791,7 @@ mseq_open(struct cdev *i_dev, int flags, int mode, struct thread *td)
 			}
 		}
 	}
+	sx_xunlock(&midistat_lock);
 
 	timer_setvals(scp, 60, 100);