fix(device): reattach the kernel driver from outside the dying process - #9
Merged
Merged
Conversation
A holder killed with SIGKILL/SIGSEGV/SIGABRT runs no user code, so uvc_close() never runs and both UVC interfaces stay driver=NONE indefinitely. Fixed by forking a helper on first interface claim that wakes on the pipe EOF the kernel delivers unconditionally when the claiming process dies, and reissues the USBDEVFS_CONNECT that process would have made. Quarantine paths leave the guard armed instead of releasing (which would recreate the eviction defect or race uvcvideo's probe), turning a quarantine into a process-lifetime-bounded leak rather than a wedged camera. Gated by LIBUVC_REATTACH_GUARD (default ON, Linux only). 6 new regression cases (5 real SIGKILL against a forked victim); CI runs the OFF variant too so a rollback stays a real rollback. Board-Evidence-SHA256: 2dd90033fbd672b74a1f277890a216e3d152e9558d2857422f43d86dd40c51c0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
libuvcdetaches the kerneluvcvideodriver when it claims a UVC interface, and hands itback only from
uvc_close(). Any exit that skips that teardown —SIGKILL,SIGSEGV, thesystemd watchdog's
SIGABRT— leaves both the VideoControl and VideoStreaming interfaces atdriver = NONEpermanently, with no self-recovery. The camera is gone until someonemanually writes to
/sys/bus/usb/drivers/uvcvideo/bind.This moves the reattach out of the process that has to die for the bug to happen.
uvc_claim_if()double-forks a small helper and keeps the write end of a pipe. The helperblocks on
read(); the wakeup is the EOF the kernel delivers when the last write end closes,and the kernel closes a dying process's descriptors unconditionally, whatever killed it. On
wake the helper reopens the device by usbfs path, verifies the device descriptor still
matches (bus addresses get reused), and issues the
USBDEVFS_CONNECTthatlibusb_attach_kernel_driver()would have issued.The load-bearing property: the code that repairs the binding is not in the process that
died, so the manner of death is irrelevant.
The quarantine paths in
uvc_close()deliberately leave the guard armed and undestroyedrather than releasing. Releasing there cannot work — with a quarantined status transfer,
handing back VideoControl lets the next URB resubmission re-claim through usbfs and evict
the driver again (the defect #7 fixed); with a quarantined stream, releasing VideoControl
alone makes
uvcvideoprobe while usbfs still holds the streaming interfaces, and itregisters no video node at all. Leaving the guard armed turns a quarantine into a leak
bounded by the process lifetime instead of a wedged camera.
Gated by
LIBUVC_REATTACH_GUARD(defaultON, Linux only); every entry point tolerates aNULLguard, so a platform or build without it behaves exactly as before.Why
Root-caused, not guessed. The trigger reproduced 10/10 across three unrelated holder
processes — including a bare
gst-launch-1.0 libuvch264srcwith no engine involved — whilethe counterfactual passed:
SIGTERMon the identical held state recovered 3/3, ~94 normalclaim/release cycles produced zero wedges, and a v4l2-path camera under the identical
SIGKILLnever wedges because it never had a detach to undo.This is not a theoretical hardening. The systemd watchdog already fires
SIGABRTonframes-not-advancing, so the documented watchdog → wedge → restart loop is this defect,self-triggering, with no human in the loop — and systemd reports the unit
activewhile thecamera is missing.
A purely userspace fix in the close path is provably insufficient here:
SIGKILLexecutes nouser code at all.
How to verify
SIGKILLagainsta forked victim process rather than simulating one, and
libuvc.teardown.quarantined_handle_stays_armedfails if anyone "tidies up" the quarantinebranch by disarming it.
-DLIBUVC_REATTACH_GUARD=OFFkeeps all 27 pre-existing cases green, and CI asserts theinventory is exactly 27 with no
libuvc.reattach.*, so a rollback is a real rollback andnot a quietly smaller gate.
ones.
Verified on real RK3588 hardware with a DJI Osmo Pocket 3 before this PR was opened. Both
previously-wedging cells were re-run ×5 each, killing only once the holder was observed
holding the interface (
driver=usbfsand an open fd on the device's usbfs node — ablind-timer kill can pass by accident and has):
gst-launch-1.0 libuvch264src, SIGKILLedcerastreamSIGKILLed mid-streamMedian reattach 34 ms, and no manual rebind was needed before any attempt. The kernel
journal independently corroborates it: a
usb 5-1: Found UVC 1.00 devicere-probe lands6–19 ms after every one of the ten kills, with nobody issuing a bind command.
Risks
libusb_claim_interface()returns, so a kill landing in the detach→claim gap (tens ofmilliseconds, once per claim) still wedges — reproduced 3/3 on hardware, where the
helper-process probe correctly found no helper. That is a narrower remnant of the same
window, not a regression: the behaviour there is exactly what it is on
maintoday.Closing it means arming before the detach and disarming on claim failure, which is a
separate, deliberate change.
hand-rolled digit formatting instead of
snprintf, andpoll()instead ofnanosleep()(the teardown suite
--wrapsnanosleep, and a forked helper must not fall into a test'ssynthetic event thread).
reuse but strictly worse on failure: a stuck helper would pin the claim and create an
unrecoverable wedge. Reopen-by-path plus an identity check means a dead helper leaves
today's behaviour and never something worse.
EBUSYretry loop is required, not polish: the helper wakes during the dyingprocess's
exit_files(), and until the usbfs fd is among the closed ones the kernel answersEBUSY. A single attempt loses that race silently.