Details

Reviewers

markj
emaste
dev_submerge.ch
meka_tilda.center
imp
bapt

Commits

rGd692c314d29a: sound: Implement asynchronous device detach
rG44e128fe9d92: sound: Implement asynchronous device detach
rG9eff58c6d52b: sound: Implement asynchronous device detach

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 55799
Build 52688: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

christos edited the summary of this revision. (Show Details)Jan 24 2024, 3:40 PM

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.

Harbormaster completed remote builds in B55592: Diff 133277.Jan 24 2024, 3:42 PM

@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 added inline comments.Jan 24 2024, 3:45 PM

sys/dev/sound/pcm/sound.c
1192–1196	Do we need the `while` here or can it just be an `if`?

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

sys/dev/sound/clone.c
400	Kernel panic here, but only after I killed Jack: panic: mutex clone lock not owned Again running Jack, no additional sysctl settings. Pull the USB plug, uaudio device is detached. Jack hangs after a `poll()` timeout, sigwait process state. 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?

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.

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.

Implement possible fix for Florian's panic.

Harbormaster completed remote builds in B55676: Diff 133487.Jan 29 2024, 2:14 PM

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

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

sys/dev/sound/clone.c
410	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()`.

Fix lock-related warnings and panics.

Harbormaster completed remote builds in B55714: Diff 133614.Jan 30 2024, 5:21 PM

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:

Run Jack on a USB audio device
Unplug device, Jack stops and hangs
Replug device, driver does not attach
Kill Jack, driver attaches device

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

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
515	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
394–395	If this isn't needed anymore, it should be removed as a separate patch. This diff is getting too big.
1215	Sorry, I don't really follow this. Could you please explain it in a code comment?

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:

Run Jack on a USB audio device

Unplug device, Jack stops and hangs

Replug device, driver does not attach

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.

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 added a parent revision: D43737: sound: remove snddev_info->inprog and pcm_inprog().Feb 4 2024, 3:06 PM

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

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

Harbormaster completed remote builds in B55795: Diff 133838.Feb 4 2024, 3:29 PM

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.

Harbormaster completed remote builds in B55799: Diff 133842.Feb 4 2024, 6:02 PM

christos marked an inline comment as done.Feb 4 2024, 6:02 PM

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.

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>

Harbormaster completed remote builds in B55812: Diff 133873.Feb 5 2024, 1:50 PM

christos retitled this revision from WIP: sound: implement asynchronous detaching to WIP: sound: implement asynchronous device detach.Feb 5 2024, 1:53 PM

christos edited the summary of this revision. (Show Details)

christos added a reviewer: imp.Feb 5 2024, 3:35 PM

markj added inline comments.Feb 6 2024, 4:24 PM

sys/dev/sound/clone.c
374	Which lock protects this list? Don't you need the PCM lock?
sys/dev/sound/pcm/mixer.c
861	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.

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
370	I'd assert that ce->refcount == 0
370–376	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
1186	I'd set the DEAD flag before doing the broadcast...
sys/dev/sound/usb/uaudio.c
1253	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 marked 3 inline comments as done.Feb 8 2024, 12:20 PM

christos added inline comments.

sys/dev/sound/clone.c
370	`snd_clone_entry` doesn't have a `refcount`field. 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.
374	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
861	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
1253	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.

christos marked an inline comment as done.Feb 8 2024, 12:21 PM

markj added inline comments.Feb 11 2024, 4:37 PM

sys/dev/sound/clone.c
374	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
861	Does an open fd block destroy_dev()? I thought it did not.

verm_darkbeer.org added a subscriber: verm_darkbeer.org.Feb 13 2024, 4:03 PM

Apply on top of D44411.

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

Harbormaster completed remote builds in B56783: Diff 136215.Mar 26 2024, 5:49 AM

christos mentioned this in D11140: OSS: allow unplug sound cards without apps close devices.Mar 26 2024, 5:49 AM

christos edited the summary of this revision. (Show Details)

christos edited parent revisions, added: D44411: sound: Get rid of snd_clone and use DEVFS_CDEVPRIV(9); removed: D43737: sound: remove snddev_info->inprog and pcm_inprog().

Fixed kern_poll() panic. Depends on D44544.

christos added a parent revision: D44544: sound: Drain buffer selinfo in sndbuf_free().Mar 28 2024, 5:17 AM

christos retitled this revision from sound: implement asynchronous device detach to sound: Implement asynchronous device detach.Mar 28 2024, 5:19 AM

markj accepted this revision.Mar 30 2024, 4:08 PM

This revision is now accepted and ready to land.Mar 30 2024, 4:08 PM

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

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.

Closed by commit rG9eff58c6d52b: sound: Implement asynchronous device detach (authored by christos). · Explain WhyMar 31 2024, 2:18 PM

This revision was automatically updated to reflect the committed changes.

christos added a commit: rG9eff58c6d52b: sound: Implement asynchronous device detach.

christos added a reverting change: rG365067e98e4e: Revert "sound: Implement asynchronous device detach".Apr 5 2024, 10:39 AM

christos reopened this revision.Apr 11 2024, 4:12 PM

Re-opening since D44411 seems to be fixed.

Harbormaster completed remote builds in B57039: Diff 136898.Apr 11 2024, 4:12 PM

christos added a reviewer: bapt.Apr 11 2024, 4:13 PM

christos removed a parent revision: D44544: sound: Drain buffer selinfo in sndbuf_free().