sound: Fix lock order reversals in mseq_open()

[christos]

Summary

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

[emaste]

This pattern can be an area for concern as elements of scp may be modified while the lock is dropped. For instance, could scp->maxunits have a different value after dropping and retaking the lock?

[markj]

In D45770#1044096, @emaste wrote:

This pattern can be an area for concern as elements of scp may be modified while the lock is dropped. For instance, could scp->maxunits have a different value after dropping and retaking the lock?

Yes, we cannot address LORs by simply dropping a lock, reacquiring it, and calling it fixed. Some analysis needs to be done to prove that such a change is correct, or a more extensive code change is needed.

sys/dev/sound/midi/sequencer.c

773

midimapper_open() is a weird function, it just counts the number of entries in the midi_devs list. Is there any reason we can't call it earlier, before acquiring seq_lock?

Or, can we introduce _locked variants of midimapper_open() and midimapper_fetch_synth() which assume that the lock is already held, and then modify this code to lock the midi state and use the _locked variants?

[christos]

Address Ed's and Mark's comments. Sorry for the lack of analysis. The _locked
variants approach Mark proposed works better. Calling midimapper_open() before
we lock seq_lock could work, but midimapper_fetch_synth() would need a _locked
variant anyway.

[christos]

sys/dev/sound/midi/midi.h

44

Instead of exposing the lock like this I could also implement lock/unlock functions, but I don't see a good reason for doing this.

[markj]

sys/dev/sound/midi/midi.h

44

IMO it is usually better to hide the lock behind a function interface so that it's easier to change the lock type later without having to update all consumers, but it's up to you.

sys/dev/sound/midi/sequencer.c

810

You can release this lock earlier, there's no harm in that (since unlocking a mutex never blocks). I think it can be released right after the loop.

[christos]

Move sx_xunlock() after the loop.

[christos]

sys/dev/sound/midi/midi.h

44

Since midi/midi.c also uses sx_x[un]lock() directly, I will keep it like this for now and implement a function interface for both midi.c and sequencer.c in a followup patch.

[christos]

@markj D45857

[emaste]

Looks fine, but are there other consumers of midimapper_open and midimapper_fetch_synth? Perhaps you can just replace the lock acquisition in those functions with the assertion, effectively providing only the locked variant?

[christos]

In D45770#1046334, @emaste wrote:

Looks fine, but are there other consumers of midimapper_open and midimapper_fetch_synth? Perhaps you can just replace the lock acquisition in those functions with the assertion, effectively providing only the locked variant?

There are no other consumers of either function yet.

[emaste]

There are no other consumers of either function yet.

OK. It's fine this way if you want (so I clicked accept), but I'd suggest providing only the locked variant for now.