sound: Implement asynchronous device detach

[christos]

Summary

Hot-unplugging a sound device, such as a USB sound card, whilst being
consumed by an application, results in an infinite loop until either the
application closes the device's file descriptor, or the channel
automatically times out after hw.snd.timeout seconds. In the case of a
detach however, the timeout approach is still not ideal, since we want
all resources to be released immediatelly, without waiting for N seconds
until we can use the bus again.

The timeout mechanism works by calling chn_sleep() in chn_read() and
chn_write() (see pcm/channel.c) in order to send the thread to sleep,
using cv_timedwait_sig(). Since chn_sleep() sets the CHN_F_SLEEPING flag
while waiting for cv_timedwait_sig() to return, we can test this flag in
pcm_unregister() (called during detach) and wakeup the sleeping
thread(s) to immediately kill the channel(s) being consumed.

Sponsored by: The FreeBSD Foundation
MFC after: 3 weeks
PR: 194727

[christos]

• Get rid of the pause(), use a CV instead and use it through snd_clone_sleep() and snd_clone_wakeup().
• Fix PCM_RELEASE_QUICK() bug?
• Remove snddev_info's inprog field as it's no longer used.

[christos]

@dev_submerge.ch Do you still get the panic with the new diff?

Also now the device detaches instantly since we no longer use pause().

[christos]

sys/dev/sound/pcm/sound.c

1152–1153

Do we need the while here or can it just be an if?

[dev_submerge.ch]

Jackpot again! ;-)
This time it's a different problem, see inline comment.

sys/dev/sound/clone.c

403 ↗

(On Diff #133277)

Kernel panic here, but only after I killed Jack:

panic: mutex clone lock not owned

Again running Jack, no additional sysctl settings.

1. Pull the USB plug, uaudio device is detached.
2. Jack hangs after a poll() timeout, sigwait process state.
3. I kill the jackd process, kernel panic occurs.

From stacktrace the call comes from devfs_close() -> dsp_close() -> snd_clone_unref(). Should the clones still be accessible after the uaudio device is detached?

[dev_submerge.ch]

FYI, if I disable vchans on the device, the mutex clone kernel panic also occurs when I just stop Jack as usual. Without forcing a device detach by pulling the USB plug. It seems like the mutex owner assertions in snd_clone_wakeup() are violated even without running the pcm_unregister() code first. Hope this helps.

[christos]

In D43545#995158, @dev_submerge.ch wrote:

FYI, if I disable vchans on the device, the mutex clone kernel panic also occurs when I just stop Jack as usual. Without forcing a device detach by pulling the USB plug. It seems like the mutex owner assertions in snd_clone_wakeup() are violated even without running the pcm_unregister() code first. Hope this helps.

I forgot to handle the case where snd_clone_wakeup() can be called whilst being attached. I'll upload the fix later today.

Should the clones still be accessible after the uaudio device is detached?

No, they shouldn't. What seems to be happening is that pcm_unregister() waits until the application (in this case jackd, and also mpv that I noticed) closes the FD so that it can continue, which apparently never happens. This is obviously an application error, but we must somehow forcibly free the clones (maybe something similar to what this patch does for channels being referenced during detach). I am trying to come up with a solution today.

[christos]

Implement possible fix for Florian's panic.

[dev_submerge.ch]

The case without pulling the plug first: When I stop Jack, there's no kernel panic now, but I get the following messages:

Jan 30 13:59:09 current kernel: exclusive sleep mutex clone lock (clone manager) r = 0 (0xfffff8011ac4d2e0) locked @ /usr/src/sys/dev/sound/clone.c:390
Jan 30 13:59:09 current kernel: stack backtrace:
Jan 30 13:59:09 current kernel: #0 0xffffffff80bc7c15 at witness_debugger+0x65
Jan 30 13:59:09 current kernel: #1 0xffffffff80bc8d79 at witness_warn+0x3e9
Jan 30 13:59:09 current kernel: #2 0xffffffff80adf0fe at destroy_dev+0x1e
Jan 30 13:59:09 current kernel: #3 0xffffffff80885823 at snd_clone_gc+0x223
Jan 30 13:59:09 current kernel: #4 0xffffffff80885b82 at snd_clone_unref+0x52
Jan 30 13:59:09 current kernel: #5 0xffffffff808cf44b at dsp_close+0x73b
Jan 30 13:59:09 current kernel: #6 0xffffffff809db9bf at devfs_close+0x48f
Jan 30 13:59:09 current kernel: #7 0xffffffff8112e002 at VOP_CLOSE_APV+0x32
Jan 30 13:59:09 current kernel: #8 0xffffffff80c640a0 at vn_close1+0x100
Jan 30 13:59:09 current kernel: #9 0xffffffff80c626af at vn_closefile+0x3f
Jan 30 13:59:09 current kernel: #10 0xffffffff809dc37b at devfs_close_f+0x2b
Jan 30 13:59:09 current kernel: #11 0xffffffff80aece5b at _fdrop+0x1b
Jan 30 13:59:09 current kernel: #12 0xffffffff80af06c3 at closef+0x1e3
Jan 30 13:59:09 current kernel: #13 0xffffffff80af45e6 at closefp_impl+0x76
Jan 30 13:59:09 current kernel: #14 0xffffffff81058463 at amd64_syscall+0x153
Jan 30 13:59:09 current kernel: #15 0xffffffff8102ab1b at fast_syscall_common+0xf8

Jan 30 13:59:09 current kernel: uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
Jan 30 13:59:09 current kernel: exclusive sleep mutex clone lock (clone manager) r = 0 (0xfffff8011ac4d2e0) locked @ /usr/src/sys/dev/sound/clone.c:390
Jan 30 13:59:09 current kernel: stack backtrace:
Jan 30 13:59:09 current kernel: #0 0xffffffff80bc7c15 at witness_debugger+0x65
Jan 30 13:59:09 current kernel: #1 0xffffffff80bc8d79 at witness_warn+0x3e9
Jan 30 13:59:09 current kernel: #2 0xffffffff80ee63e4 at uma_zalloc_debug+0x34
Jan 30 13:59:09 current kernel: #3 0xffffffff80ee5ef7 at uma_zalloc_arg+0x27
Jan 30 13:59:09 current kernel: #4 0xffffffff80b25c1e at malloc+0x7e
Jan 30 13:59:09 current kernel: #5 0xffffffff80adf6c4 at destroy_devl+0x3b4
Jan 30 13:59:09 current kernel: #6 0xffffffff80adf120 at destroy_dev+0x40
Jan 30 13:59:09 current kernel: #7 0xffffffff80885823 at snd_clone_gc+0x223
Jan 30 13:59:09 current kernel: #8 0xffffffff80885b82 at snd_clone_unref+0x52
Jan 30 13:59:09 current kernel: #9 0xffffffff808cf44b at dsp_close+0x73b
Jan 30 13:59:09 current kernel: #10 0xffffffff809db9bf at devfs_close+0x48f
Jan 30 13:59:09 current kernel: #11 0xffffffff8112e002 at VOP_CLOSE_APV+0x32
Jan 30 13:59:09 current kernel: #12 0xffffffff80c640a0 at vn_close1+0x100
Jan 30 13:59:09 current kernel: #13 0xffffffff80c626af at vn_closefile+0x3f
Jan 30 13:59:09 current kernel: #14 0xffffffff809dc37b at devfs_close_f+0x2b
Jan 30 13:59:09 current kernel: #15 0xffffffff80aece5b at _fdrop+0x1b
Jan 30 13:59:09 current kernel: #16 0xffffffff80af06c3 at closef+0x1e3
Jan 30 13:59:09 current kernel: #17 0xffffffff80af45e6 at closefp_impl+0x76

[dev_submerge.ch]

Unfortunately I still get the kernel panic when I pull the USB plug, see inline comments.

sys/dev/sound/clone.c

413 ↗

(On Diff #133487)

Kernel panic still happens here with Jack running and then pulling the plug:

panic: mutex clone lock not owned

Stacktrace is also the same, devfs_close() -> dsp_close() -> snd_clone_unref().

[christos]

Fix lock-related warnings and panics.

[dev_submerge.ch]

As communicated in private, the latest update to this change fixes all kernel panics and witness complaints.

I did some more tests. It looks like a "malicious" application (e.g. Jack) can still prevent a detached device from attaching again:

1. Run Jack on a USB audio device
2. Unplug device, Jack stops and hangs
3. Replug device, driver does not attach
4. Kill Jack, driver attaches device

Is this to be expected or should this issue be fixed?

[markj]

From my reading of the diff, you added a new mutex to the cloner. Existing code mostly uses the PCM lock to serialize accesses to the cloner, so this is confusing - what is the division of responsibility between the PCM mutex and the cloner mutex?

I think you added the mutex because you wanted to add a CV to the cloner, and to synchronize a CV you need a mutex. What you could do instead is pass a pointer to the mutex to snd_clone_sleep(). That is, add #define PCM_LOCKPTR(d) (&(d)->lock), and call snd_clone_sleep(d->clones, PCM_LOCKPTR(d)).

For this to work, you need a way to drop the PCM lock around calls to the cloner garbage collector. I suggested a way to do that in an inline comment.

sys/dev/sound/clone.c

546 ↗

(On Diff #133614)

Can we perform garbage collection asynchronously? That is, what if snd_clone_gc() uses destroy_dev_sched_cb() instead of destroy_dev()? Then you can probably avoid dropping the PCM lock. I'm pretty sure that would work, and it makes the locking much simpler.

sys/dev/sound/pcm/sound.c

501–502

If this isn't needed anymore, it should be removed as a separate patch. This diff is getting too big.

1155

Sorry, I don't really follow this. Could you please explain it in a code comment?

[christos]

In D43545#996250, @dev_submerge.ch wrote:

As communicated in private, the latest update to this change fixes all kernel panics and witness complaints.

I did some more tests. It looks like a "malicious" application (e.g. Jack) can still prevent a detached device from attaching again:

1. Run Jack on a USB audio device
2. Unplug device, Jack stops and hangs
3. Replug device, driver does not attach
4. Kill Jack, driver attaches device

Is this to be expected or should this issue be fixed?

This is an (and possible the last) issue this patch has to address. So what happens is, when you unplug the device while jack (and mpv that I noticed) is consuming it, it still keeps the descriptor open and tries to call ioctls on it (see the errors from mpv when you hot-unplug), which means that there are still referenced clones, and as a result pcm_unregister() will block in snd_clone_sleep() until the clone is unreferenced and pcm_unregister() can go ahead to destroy its devfs node. The obvious solution would be to simply ignore the fact that we still have busy clones, and just force-destroy the devfs nodes, but this results in a use-after-free panic because jack will reference freed memory if we free the descriptor and jack tries to use it. This is definitely an application issue (i.e the apps should close the descriptors after failure), but we have to somehow fix this. I am still trying to figure out a solution to this.

[theron.tarigo_gmail.com]

In D43545#996996, @christos wrote:

This is an (and possible the last) issue this patch has to address. So what happens is, when you unplug the device while jack (and mpv that I noticed) is consuming it, it still keeps the descriptor open and tries to call ioctls on it (see the errors from mpv when you hot-unplug), which means that there are still referenced clones, and as a result pcm_unregister() will block in snd_clone_sleep() until the clone is unreferenced and pcm_unregister() can go ahead to destroy its devfs node. The obvious solution would be to simply ignore the fact that we still have busy clones, and just force-destroy the devfs nodes, but this results in a use-after-free panic because jack will reference freed memory if we free the descriptor and jack tries to use it. This is definitely an application issue (i.e the apps should close the descriptors after failure), but we have to somehow fix this. I am still trying to figure out a solution to this.

The kernel cannot kill the descriptors, only the application may release them.
If a buggy application does not release them, it should be a resource leak only for the lifetime of that application process.

Clones of destroyed devices should enter an "orphan" state which is not destroyed but has no associated hardware device, awaiting cleanup when descriptor to it is closed.

I suggest to look into how this problem is already solved for USB storage block devices, where an application's refusal to close a fd does not interfere with subsequent device attachment and devfs node creation.

[christos]

Address most of Mark's comments. Still some pending.

Split inprog removal into its own patch. Depends on D43737.

[christos]

Handle busy mixer:

• Write mixer_sleep() and mixer_wakeup(), same logic as with snd_clone_sleep()/snd_clone_wakeup().
• Write a mixer_get_sndmixer() function to be used in pcm_unregister() in order to call mixer_sleep().
• Sleep if we detect a busy mixer in pcm_unregister().
• Wakeup when the mixer is closed.

[christos]

In D43545#997000, @theron.tarigo_gmail.com wrote:

In D43545#996996, @christos wrote:

This is an (and possible the last) issue this patch has to address. So what happens is, when you unplug the device while jack (and mpv that I noticed) is consuming it, it still keeps the descriptor open and tries to call ioctls on it (see the errors from mpv when you hot-unplug), which means that there are still referenced clones, and as a result pcm_unregister() will block in snd_clone_sleep() until the clone is unreferenced and pcm_unregister() can go ahead to destroy its devfs node. The obvious solution would be to simply ignore the fact that we still have busy clones, and just force-destroy the devfs nodes, but this results in a use-after-free panic because jack will reference freed memory if we free the descriptor and jack tries to use it. This is definitely an application issue (i.e the apps should close the descriptors after failure), but we have to somehow fix this. I am still trying to figure out a solution to this.

The kernel cannot kill the descriptors, only the application may release them.
If a buggy application does not release them, it should be a resource leak only for the lifetime of that application process.

Clones of destroyed devices should enter an "orphan" state which is not destroyed but has no associated hardware device, awaiting cleanup when descriptor to it is closed.

I suggest to look into how this problem is already solved for USB storage block devices, where an application's refusal to close a fd does not interfere with subsequent device attachment and devfs node creation.

Working on it. Thank you.

[christos]

Simplify diff a lot. Allow device to detach asynchronously and leave consumed
devfs node(s) (both clone and mixer) in an orphan state until they are cleaned
up by destroy_dev_sched_cb().

• Rewrite snd_clone_destroy(). Call destroy_dev_sched_cb() on all clones.
• Get rid of the EBUSY return in mixer_uninit() and also use destroy_dev_sched_cb().
• Get rid of all sleep/wakeup mechanisms implemented in the previous diffs. We no longer need to wait for devfs-node unref.

Did the following tests:

• cat /dev/random >/dev/dsp and unplug: succeeds.
• mpv foo.mp3 and unplug: suceeds.
• mpv foo.mp3 and unplug and re-attach: suceeds.
• mpv foo.mp3 and kldunload snd_uaudio on another terminal: succeeds.
• jackd -R -d oss -r 48000 -C /dev/dsp and unplug: panics during a poll (probably), but otherwise "succeeds".

The jackd panic:

panic: ASan: Invalid access, 8-byte read at 0xfffffe0046de4398, UMAUseAfterFree(fd)
cpuid = 1
time = 1707145812
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe0046b353d0
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe0046b35530
vpanic() at vpanic+0x1fd/frame 0xfffffe0046b356d0
panic() at panic+0xb5/frame 0xfffffe0046b35790
kasan_code_name() at kasan_code_name/frame 0xfffffe0046b35860
selfdfree() at selfdfree+0x169/frame 0xfffffe0046b358b0
kern_poll_kfds() at kern_poll_kfds+0xb26/frame 0xfffffe0046b35a50
kern_poll() at kern_poll+0x15a/frame 0xfffffe0046b35c50
sys_poll() at sys_poll+0x102/frame 0xfffffe0046b35d10
amd64_syscall() at amd64_syscall+0x365/frame 0xfffffe0046b35f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe0046b35f30
--- syscall (209, FreeBSD ELF64, poll), rip = 0x827d3f71a, rsp = 0x8315abd88, rbp = 0x8315abdb0 ---
KDB: enter: panic
[ thread pid 929 tid 100170 ]
Stopped at      kdb_enter+0x34: movq    $0,0x1ef5cc1(%rip)
db>

[markj]

sys/dev/sound/clone.c

364 ↗

(On Diff #133873)

Which lock protects this list? Don't you need the PCM lock?

sys/dev/sound/pcm/mixer.c

838

Why does the destruction have to happen asynchronously in this case? The reason for dropping the snd lock is because destroy_dev() may sleep, but you could also just move the destroy_dev() call down as you did here.

[imp]

This generally looks close to what you'd want. you want some way to stick around while things are referenced, but you also want to install something for higher layers that want to access the device. I think doing the destroy device with scheduling a callback when it's done would do that correctly. I have a couple of specific questions, mostly due to my unfamiliarity with the code.

sys/dev/sound/clone.c

360 ↗

(On Diff #133873)

I'd assert that ce->refcount == 0

380 ↗

(On Diff #133873)

I'd consider setting dev->si_drv2 to NULL here. I don't see how it gets set to NULL other

sys/dev/sound/pcm/sound.c

1147

I'd set the DEAD flag before doing the broadcast...

sys/dev/sound/usb/uaudio.c

1258

any locking considerations for pcm_registered here? Could it be set and then another thread calls pcm_unregister in parallel? Ditto below for mixer

[christos]

sys/dev/sound/clone.c

360 ↗

(On Diff #133873)

snd_clone_entry doesn't have a refcountfield. This is part of the clone manager struct (snd_clone) to keep track of how many of its clones are referenced. Individual clones, after all, can only be open by a single thread at a time, so it doesn't make much sense to keep a refcount for them.

364 ↗

(On Diff #133873)

This function might get called after we've returned from pcm_unregister() (and thus freed the PCM lock). Maybe we need to add an SX lock to snd_clone and use it to serialize access here?

sys/dev/sound/pcm/mixer.c

838

As is the case for DSP cdevs, an application might keep a /dev/mixer FD open when we are detaching, so we need a similar mechanism here as well.

sys/dev/sound/usb/uaudio.c

1258

This pcm_registered flag is internal to the uaudio driver (not to be confused with PCM framework's SD_F_REGISTERED) and is only actually useful when we need to call uaudio_detach_sub() from uaudio_attach_sub() during an attach failure. In normal circumstances (i.e, we've got past the attach phase) this flag will always be true.

[markj]

sys/dev/sound/clone.c

364 ↗

(On Diff #133873)

Well, sx is probably not the right lock type. A mutex would be better for synchronizing the clone list.

Hmm. If we're here, then the clone structure has been detached from the device and we're just waiting for the refcount to go to 0. That wasn't really clear to me before. So this code is probably ok since the callbacks will be serialized.

There are still some races in this code. For instance, dsp_close() calls snd_clone_unref() without any locks held. I can't see how that's safe. It can probably be handled in a separate diff though.

sys/dev/sound/pcm/mixer.c

838

Does an open fd block destroy_dev()? I thought it did not.

[christos]

Apply on top of D44411.

The kern_poll() panic mentioned in a previous comment still persists. I am
working to fix it.

[christos]

Fixed kern_poll() panic. Depends on D44544.

[christos]

@dev_submerge.ch @emaste Do you think this is good to go?

[dev_submerge.ch]

Did another test drive with all patches together, works fine now. Sorry for the long "test cycle", I really have to get a USB expansion card for bhyve passthrough, so I don't have to reboot into CURRENT all the time.

[christos]

Re-opening since D44411 seems to be fixed.

[bapt]

tested with success on my laptop