fix(device): hand the kernel driver back when the claim fails - #10
Merged
Conversation
uvc_claim_if() detaches the kernel driver and claims the interface in two separate kernel calls, and rolled back neither. A detach that lands followed by a claim that fails -- a busy device, a kernel racing the same interface -- left the interface bound to nothing, in a process that is alive and well. Nothing came back for it either: uvc_release_if() returns early for any interface absent from devh->claimed, so the driver stayed off for the rest of the process's life. Measured on an RK3588 board as 5-1:1.0=usbfs, 5-1:1.1=NONE persisting 83 seconds with nothing killed. The claims are split across two moments -- VideoControl at uvc_open(), VideoStreaming later at stream negotiation -- which widens the window in practice. uvc_claim_if() now undoes its own detach with the libusb_attach_kernel_driver() the release path would have made. The caller still receives the claim's error; what happened to the driver afterwards is not the caller's business. The reattach backstop's arming moved with it. An interface is armed BEFORE the detach rather than after a successful claim, because it is the detach, not the claim, that makes this process the reason the interface is unbound. It is disarmed again on every path out that does not end in a claim: a detach that failed took nothing, and a claim that failed is disarmed only once the driver is genuinely back -- a hard reattach failure stays armed so the helper repairs it from outside. That predicate is uvc_release_if()'s, unchanged, so there is still exactly one definition of "the kernel has this interface back". reattach_guard.c is untouched: todo 5's arm/disarm API was already sufficient and only its call sequence moved, so the forked helper's async-signal-safety is not reachable from this change. With LIBUVC_REATTACH_GUARD=OFF the reattach still runs and only the bookkeeping compiles out, so the rollback build gets the repair without the backstop. Three regression cases, all red first: claim_failure_after_detach_reattaches the defect itself failed_reattach_after_failed_claim_stays_armed the backstop half detach_failure_leaves_nothing_armed the risk arming early adds claiming_an_interface_arms_the_guard keeps every assertion it had and gains three that read the armed mask from inside the detach call, which is the only way to tell "armed before the detach" from "armed after the claim".
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
uvc_claim_if()now hands the kernel driver back when the claim fails, and the reattachbackstop arms at the detach instead of after a successful claim.
Why
Detaching the kernel driver and claiming the interface are two separate kernel calls, and
this function rolled back neither. A detach that lands followed by a claim that fails — a
busy device, a kernel racing the same interface — left the interface bound to nothing:
What makes that terminal rather than transient is that nothing ever comes back for it:
uvc_release_if()returns early for any interface absent fromdevh->claimed, so the driverstays off for the rest of the process's life. No kill and no crash are involved — a
completely healthy process ends up permanently one interface short. Measured on an RK3588
board as
5-1:1.0=usbfs, 5-1:1.1=NONEpersisting 83 seconds with nothing killed. Thewindow is wider in practice than the code suggests, because the claims are split across two
moments:
uvc_open()claims VideoControl, and VideoStreaming is claimed later at streamnegotiation.
This is a different defect from the one #9 fixed. #9 covers a process that dies without
running its cleanup. This one is a process that runs everything and still strands the
interface.
How
Two separable changes:
The repair, in-process. A claim that fails is followed by the
libusb_attach_kernel_driver()the release path would have made. The caller still receivesthe claim's error — what happened to the driver afterwards is not the caller's business.
This works in both build variants; with
LIBUVC_REATTACH_GUARD=OFFthe repair still runsand only the bookkeeping below compiles out.
The bookkeeping. An interface is armed before the detach, because it is the detach
— not the claim — that makes this process the reason the interface is unbound. It is
disarmed again on every path out that does not end in a claim:
devh->claimedattach_kernel_driverUVC_SUCCESSThe disarm predicate is
uvc_release_if()'s, unchanged —SUCCESS,NOT_FOUNDandNOT_SUPPORTEDall mean the kernel has the interface back — so there is still exactly onedefinition of that in the codebase.
src/reattach_guard.candinclude/libuvc/reattach_guard.hare untouched. #9's arm/disarmAPI was already sufficient; only the call sequence in
device.cmoved. That is deliberate:no helper-side line was added or reordered, so the forked helper's async-signal-safety is not
reachable from this change, and the review surface is one function.
Three new regression cases, all reproducing the defect before the fix:
claim_failure_after_detach_reattaches— the defect itself (attach_calls == 0pre-fix)failed_reattach_after_failed_claim_stays_armed— the backstop half: if the in-processrepair also fails, the interface must stay armed
detach_failure_leaves_nothing_armed— a guard for the risk arming early adds: aninterface armed that libuvc then never takes
claiming_an_interface_arms_the_guardkeeps all six of its original assertions and gainsthree that read the armed mask from inside the
--wrapped detach call — the only way todistinguish "armed before the detach" from "armed after the claim". Nothing it verified was
weakened.
How to verify
To see the defect, check out this branch, revert
src/device.conly, rebuild, and run-R 'libuvc\.reattach\.': cases 32, 33 and 34 go red atguard_existed_at_detach[0] == 1and
attach_calls == 1.detach_failure_leaves_nothing_armedis green both before and after by design — it guards thefix, it does not reproduce the bug. It was proved load-bearing by mutation: delete only the
disarm on the detach-failure branch and it fails at
uvc_reattach_guard_armed_mask(...) == 0.Also verified:
-Wall -Wextraclean on both changed files, and theLIBUVC_AUTO_DETACH_KERNEL_DRIVER=OFFshared build still builds.Risks
sites. The compensating cover is that
claiming_an_interface_arms_the_guardnow asserts thetiming directly rather than only the end state, so a future regression in either direction
is caught.
so a
uvc_claim_if()that fails at the detach forks a helper it then disarms. Cost: oneprocess that wakes to an empty armed set and exits.
uvc_free_devh()destroys it as before.libusb_attach_kernel_driver()on a VideoStreaming interface binds nothing immediately,because
uvcvideoclaims streaming interfaces as part of the VideoControl probe rather thanprobing them directly. The CONNECT still succeeds and the interface still leaves this
process's responsibility — the state becomes indistinguishable from "libuvc never touched
it" — but this does not make
/dev/videoNreappear on the spot for a VS-only failure.That is unchanged from the existing teardown ordering contract (README invariant 2).
closed. Arming before the detach does mean an interface is now armed during that window as
a side effect, but that consequence has not been re-measured on hardware (it scored 4/4
WEDGED pre-fix), so the residual stays open and tracked exactly as it was.
--wrapharness forces the exact failure — a claim failingafter a successful detach, in a live, unkilled process — and observes the repair directly. A
board run could only re-observe a transient the harness produces on demand.
Docs updated in the same change: README (inventory 33→36, reattach group 5→8, teardown
contract invariant 5 rewritten for the new arm point plus the claim-failure reattach as a
contract term),
CHANGELOG.ceralive.md, and the CI exact-inventoryjqassertion.