Fix Crosvm USB device tracking

[vadika]

Summary

Track Crosvm USB devices by guest port instead of VID/PID. Persist and validate the port mapping across daemon and VM restarts, detach only the selected device, and retry temporary port exhaustion without evicting unrelated devices.

Validation

• nix fmt -- --fail-on-change
• pytest: 33 passed
• nix build .#default

Required by tiiuae/ghaf#2123.

[brianmcgillion]

Review: crosvm USB device tracking

Thanks for taking this on — tracking by guest port instead of VID/PID is the right direction, and removing the "detach everything to make room" workaround fixes a real problem. The concerns below are about the persistence layer and the new error paths rather than the core idea.

I verified the findings against 0f42be5 and reproduced the first four locally. pytest (33 tests), ruff check, ruff format --check and mypy on the changed modules all pass, and the vmm.py elif-to-early-return refactor is behaviour-preserving.

---

Blocking

1. A persisted port replayed against a restarted VM silently skips the attach
crosvmlink.py:48-56, vhotplug.py:122-139

crosvm reallocates ports from zero when the VM reboots, and nothing invalidates crosvm_usb_port_map. On restart attach_connected_usb re-runs with no state clearing, so a port from the previous VM generation is validated only by VID/PID — which by definition cannot separate two identical devices, the exact ambiguity this PR exists to remove. Driving the real CrosvmLink against a fake crosvm that allocates the lowest free port:

persisted before VM restart: {'.../001/002': 1, '.../001/003': 2}

B reattached -> reported port 1     # took lowest free, not its persisted 2
A reattached -> reported port 1     # "already attached, skipping" -- that is B
actual attach calls issued: 1       # device A was never attached

Two consequences. Device A is authorized on the host at device.py:249-256 before the attach; because the attach is skipped rather than failed, no exception fires and the deauthorize path at device.py:262-266 never runs — A is left live on the host, in no VM, while set_vm_for_device and the usb_attached notification report it as attached. That undoes the deauthorize-by-default behaviour from 7b3b5d8 / 4320deb. Second, state now records A at port 1, which is physically B, so a later detach of A detaches B.

2. usb_list() raising aborts state cleanup and host de-authorization
crosvmlink.py:167-184 -> device.py:339-357

On master a failed list logged and returned [], so the removal loop was a harmless no-op. It now raises, and on the removal path that skips everything after device.py:339: remove_vm_for_device, deauthorize_usb_device, and notify_dev_detached. The trigger is routine — crosvm usb list <socket> exits non-zero whenever the VM is down. Reachable from the unplug handler at vhotplug.py:96-99 and from _on_usb_suspend -> detach_connected_usb against a stopped VM.

No crash (both callers catch RuntimeError), but the entry leaks in both maps and is persisted to disk, and with usbAuthorization enabled a still-present device is never de-authorized on the host. That stale entry is also the precondition for the cross-VM detach in finding 5.

3. Non-atomic state writes plus an uncaught JSONDecodeError brick the daemon
devicestate.py:41-56, :58-66

Pre-existing, but this PR raises the write frequency from "explicit user actions" to "every attach and every detach", which multiplies the exposure. _save() truncates in place with no temp-file-plus-rename, and _load() catches only OSError while json.JSONDecodeError subclasses ValueError:

startup raises JSONDecodeError: Unterminated string starting at: line 1 column 41

The raise escapes DeviceState.__init__ at vhotplug.py:183, before AppContext exists, and main() catches only CancelledError / KeyboardInterrupt. With Restart=on-failure / RestartSec=5s in the unit that is an unbounded restart loop that never self-heals. Worse, the crash happens before set_usb_authorized_default(0) at line 190, but that module parameter is still 0 from the previous run and persists until reboot — so newly plugged devices are never authorized and passthrough stops working entirely.

_save() also has no exception handling at all, and OSError is not a RuntimeError, so a full or read-only /var propagates past every upstream handler and exits the daemon.

4. Non-numeric port index in usb_list crashes the daemon on unplug
crosvmlink.py:176, :182

int(data[i]) sits inside a try that catches only OSError, so usb_list advertises a RuntimeError contract while also able to raise ValueError. add_usb_device catches it; remove_usb_device does not. On unplug: vhotplug.py:97 -> device.py:339 -> ValueError -> vhotplug.py:98 catches only RuntimeError -> daemon exits. The API path happens to catch ValueError, so this reproduces only on physical unplug.

---

Should fix

5. The persistent map is keyed by an identifier the kernel recycles, and carries no VM identity
devicestate.py:20-23

device_node is /dev/bus/usb/BBB/DDD, where the devnum is reassigned on replug and reused across different physical devices. Every other persistent map in this file keys on persistent_id():

device B (serial BBBB) inherited port 4 from device A (serial AAAA)
persistent_id  A = usb-046d:c52b:AAAA   B = usb-1050:0407:BBBB

The value is also a bare int with no VM recorded, though a port is only meaningful relative to one crosvm instance. Combined with finding 2's stale entries, a device that inherits a stale node can drive remove_usb_device(known_port=N) against a different VM's socket; device.py:242-243 only warns and continues.

6. A detected mismatch is reported to the caller as a successful detach
crosvmlink.py:191-205

When the known port holds a different VID/PID the code warns and returns — indistinguishable from success. The caller then deletes both mappings and fires notify_dev_detached, so the CLI prints ok and the UI greys the device out while it is still live inside the guest, and the port mapping that was the only way to find it is gone. This is also inconsistent with line 215, which raises on the analogous legacy ambiguity.

7. The VID/PID guard is inert on the physical-unplug path
crosvmlink.py:196

BUSNUM / DEVNUM are kernel uevent properties parsed with an unguarded int() at usb.py:192-193, so they must be present on remove. ID_VENDOR_ID / ID_MODEL_ID come from udev's usb_id builtin via rules, which do not re-run on remove — they are read with nullable .get(), matching the note already in the tree at device.py:307. So on hot-unplug usb_info.vid is falsy, the guard short-circuits, and whatever occupies the stale port is detached. The guard is inert exactly where a stale port is most likely to be in play, and where it does run it compares device-supplied descriptor values. Useful as a staleness check; not a boundary.

8. A swallowed removal failure orphans the old VM's device
device.py:237-243, :269-271

When a device moves between VMs and remove_device fails, it is caught and logged, then set_crosvm_usb_port overwrites the port entry with the new VM's port. The old VM still holds the device at the old port and the only record of it is gone, so it can never be detached. This PR adds a new way for removal to raise, making the path more reachable.

---

Smaller items

• ok with an unexpected shape is retried as a failure (crosvmlink.py:87-92) — crosvm said ok, so the device is attached. Each retry attaches it again, and the final RuntimeError then de-authorizes the device on the host while crosvm holds several references.
• Booleans pass the port validation (devicestate.py:53) — isinstance(True, int) is True, so {"...": true} loads as port True, compares equal to port 1, and renders as argv "True". The 0..255 bound is also only applied on load, not on write or on int(r[1]).
• The same crash fixed twice is left in remove_usb_device_by_id (crosvmlink.py:137-138) — r[0] with no emptiness check, IndexError on empty stdout with exit code 0. The guard was added to the attach path (line 84) and the list path (line 171); detach-by-id is now the primary removal path and needs it too.
• RuntimeError("Timeout") discards the cause (crosvmlink.py:112) — six failure modes collapse into one string. A typo in general.crosvm surfaces as {"result":"failed","error":"Timeout"}, pointing at a slow VM rather than a config error. Consider keeping the last exception and raising ... from last_error.
• Every QEMU USB attach rewrites the state file (device.py:269-270) — vmm_add_device returns None for QEMU and set_crosvm_usb_port still calls _save(), so a QEMU-only deployment takes a blocking JSON rewrite on every plug for no benefit, carrying all of finding 3's exposure. The write is also blocking I/O on the event loop.
• No per-VM serialization (crosvmlink.py:188-208) — API handlers submit coroutines from their own threads, so between usb_list() and the usb detach P spawn a concurrent attach can land on P and get detached. An asyncio.Lock per VM socket would close it.
• Error text reaches API clients (crosvmlink.py:216-218 -> apiserver.py:286-288) — the message discloses that a VM holds two or more devices of a given model, and crosvm stderr is embedded verbatim. Relevant because the API can be served over vsock with allowedCids optional.
• State file permissions — the unit at nix/nixos-module.nix:85-98 sets no StateDirectory, UMask or User, so /var/lib/vhotplug/vhotplug.state is 0644. It contains vid:pid:serial values, including hardware serial numbers. StateDirectory=vhotplug + StateDirectoryMode=0700 + UMask=0077 would cover it.

---

Tests

The new tests are well-shaped — patching at the asyncio boundary is a clean seam, test_devicestate.py round-trips through a fresh DeviceState rather than inspecting the dict, and asserting on the absence of detach calls is exactly right for the no_available_port fix.

Two structural gaps:

The suite never runs in CI. nix/package.nix:25 sets doCheck = false, and flake.nix:64-66 exposes only the vhotplug-service NixOS test. CI runs nix-fast-build, so tests/ is dev-shell only and none of these tests will catch a future regression.

Mutation testing: 16 mutants, 10 survived. Coverage is crosvmlink.py 71%, devicestate.py 57%, vmm.py 25%. A literal revert of the fix is caught; every weaker form is not.

Mutation	Result
Revert core fix — match VID/PID, ignore known_port	caught
Restore the "detach everything on no_available_port" workaround	caught
Stop persisting / stop clearing crosvm_usb_ports	caught
Trust known_port without validating it	survived
Delete the VID/PID mismatch guard	survived
Detach anyway when the known port is empty	survived
Never retry	survived
device.py stops passing / storing the port	survived
vmm.py stops forwarding the port on removal	survived
Drop usb_list / attach-response validation	survived

The two files test the leaves but nothing asserts they are wired together. Highest value additions: a negative-direction test where known_port is present but holds a different device (must attach, must return the new port), a port-table-reset test for finding 1, the two mismatch-guard tests for finding 6, and a vmm_add_device test asserting the crosvm branch returns the port and the QEMU branch returns None.

---

One question

The deleted comment documented a specific field-observed failure: attaching before the guest kernel boots returns no_available_port, and continuing to retry eventually yields a permanent I/O error where passthrough stays broken until the VM reboots. The PR keeps the retry loop (5 attempts at 1s) and removes the mitigation, replacing it with a comment asserting the condition is benign.

I agree the old workaround was itself a bug and should go — it detached unrelated devices and bypassed state cleanup entirely. But was the permanent-I/O-error path retested, or is it assumed no longer to occur? Worth noting this PR adds exactly the data needed for a targeted version: on no_available_port, detach only the ports this daemon recorded for this VM. That keeps the new test green and preserves the mitigation.

---

Inline suggestions below for the mechanical fixes. Findings 1, 2 and 5 are design changes where I have sketched the direction rather than proposing a one-click patch, since the right call there is yours.

[brianmcgillion]

crosvm reported ok, so the device is attached — but an unparseable ok falls through to the retry, and each retry attaches it again, consuming another guest port. After the last attempt the RuntimeError reaches device.py:262-266, which de-authorizes the device on the host while crosvm still holds several references to it.

An ok you cannot parse is an unrecoverable success, not a retryable failure. Rather than retrying, re-list and adopt the port that appeared:

ports_before = {p for p, _, _ in devices}
# ... after an ok response that does not parse ...
for port, vid, pid in await self.usb_list():
    if port not in ports_before and vid == usb_info.vid and pid == usb_info.pid:
        return port

[brianmcgillion]

Finding 5. device_node is /dev/bus/usb/BBB/DDD, and the kernel reassigns the devnum on replug and reuses it across different physical devices — so this is an ephemeral handle being used as a persistent key. Every other persistent map in this file keys on persistent_id(). Confirmed against this branch:

device B (serial BBBB) inherited port 4 from device A (serial AAAA)
persistent_id  A = usb-046d:c52b:AAAA   B = usb-1050:0407:BBBB

The value also carries no VM identity, though a port number only means anything relative to one crosvm instance. With a stale entry from finding 2, a device can end up driving remove_usb_device(known_port=N) against a different VM's socket.

persistent_id() is not a straight swap — two identical devices without serials collapse to one key. Keying on physical topology plus identity (sys_name, e.g. 1-2.1, with vid:pid:serial) is stable and unique for both cases.

Worth asking whether this needs to persist at all: making it runtime-only like usb_device_vm_map and reconciling against a live crosvm usb list at startup would remove findings 1, 3 and 5 together. Persistence only buys daemon-restart recovery, which is exactly when the identifier is least trustworthy.

[brianmcgillion]

Blocking (finding 3), second half. _save() opens the real path with "w" on line 60, which truncates before json.dump writes anything — so any interruption leaves a partial file, which is what the _load() comment above is about. There is also no exception handling at all, and OSError is not a RuntimeError, so a full or read-only /var propagates past every upstream handler and exits the daemon.

This PR matters here because it moves _save() from "explicit user actions" to every attach (device.py:270) and every detach (devicestate.py:92), which widens the window by roughly the number of plug events on the machine.

I cannot attach an applyable suggestion because line 60 is outside this diff, but the shape is:

    def _save(self) -> None:
        if not self.persistent:
            return
        tmp_path = self.db_path.with_suffix(".tmp")
        try:
            with tmp_path.open("w", encoding="utf-8") as f:
                j = {
                    "selected_vms": self.selected_vms,
                    "disconnected_devices": list(self.disconnected_devices),
                    "crosvm_usb_ports": self.crosvm_usb_port_map,
                }
                json.dump(j, f, ensure_ascii=False, indent=2)
            tmp_path.replace(self.db_path)
        except OSError as e:
            logger.warning("Failed to save state database: %s", e)
            tmp_path.unlink(missing_ok=True)

Path.replace is atomic within a filesystem, so a reader sees either the old file or the new one. Adding f.flush() + os.fsync(f.fileno()) before the replace makes it durable across power loss too, at the cost of an import os. The except OSError is the important part regardless of the atomicity change — a persistence failure must not abort an attach that already succeeded.

[brianmcgillion]

Ordering hazard: set_crosvm_usb_port calls _save(), and _save() currently has no exception handling. If it raises OSError here the device is already attached to the VM but set_vm_for_device on the next line never runs — so ownership is never recorded and the device can never be detached. Recording ownership first makes the persistence write the failure-tolerant part.

Suggested change

	if isinstance(dev_info, USBInfo):
	app_context.dev_state.set_crosvm_usb_port(dev_info, attached_port)
	app_context.dev_state.set_vm_for_device(dev_info, vm_name)
	app_context.dev_state.set_vm_for_device(dev_info, vm_name)
	if isinstance(dev_info, USBInfo):
	app_context.dev_state.set_crosvm_usb_port(dev_info, attached_port)

Related, finding 8: when a device moves between VMs and the removal at line 240 fails, that exception is caught and logged and execution continues to here, where this line overwrites the old VM's port entry. The old VM still holds the device at the old port and the only record of it is gone, so it can never be detached. Worth either not proceeding with the attach when removal failed, or recording the orphaned (vm, port) pair for later reconciliation.

[vadika]

Addressed the review in 43c15ed:

• Scoped persisted Crosvm USB bindings to physical topology, VM, and control-socket generation, with restart invalidation.
• Made detach and error handling conservative, added per-VM serialization, and hardened state persistence.
• Enabled pytest in Nix builds and added focused regression coverage.

Validation passes: 51 pytest tests, treefmt (ruff/mypy), package build, NixOS service test, and both x86_64/aarch64 CI jobs.

I kept no_available_port as a bounded retry that does not detach existing devices. The previously reported permanent-I/O path was not hardware-retested in this revision.

[brianmcgillion]

Re-verified against 43c15ed. All blocking and high findings addressed — I re-tested each rather than reading the diff.

Confirmed fixed:

• VM-restart port replay — socket_generation plus clear_crosvm_usb_ports on restart.
• Cleanup when the VM is gone — CrosvmVMUnavailableError correctly separates "VM gone, clean up anyway" from "VM live and refused, abort". Better than what I suggested.
• State DB — tmp + fsync + replace, 0600, catches (OSError, TypeError, ValueError). A torn write and a non-object root both start clean instead of crash-looping.
• Port identity — keyed on sys_name, binding carries vm/generation/vid/pid/serial and is revalidated on read.
• Detach mismatch raises; a failed cross-VM removal now aborts the move; ok-without-port re-lists instead of re-attaching; the timeout chains its cause; the per-socket lock serialises list→act.
• CI — confirmed pytestCheckPhase runs under nix build: 51 passed. Hardening pinned by the NixOS test assertions.

Three non-blocking residuals, for whenever:

1. The VID/PID guard in remove_usb_device is still inert on udev remove events, where vid/pid are absent — so detach-by-known-port proceeds unconfirmed. Much narrower now that bindings are per physical port and per VM generation.
2. With persistency = false, a daemon restart re-attaches an already-attached device: port == known_port cannot match when known_port is None.
3. CrosvmUSBAttachStateError leaves the device in the guest with no state entry — the host deauthorizes it and nothing is left to detach later.

The new test_device.py / test_vmm.py coverage of the wiring is a good addition.

[nesteroff]

Honestly, I don't know how to properly review AI-generated PRs. It looks like several rounds of AI-generated changes and AI review feedback have added a lot of complexity without adding much value. In my opinion, the first version was actually simpler and easier to follow.

The problem this PR is trying to solve is the following: when two or more identical USB devices are plugged into the system and the user tries to detach one of them from the VM using the vhotplug API through the UI or CLI, the wrong device can be detached.

This seems like a fairly unlikely scenario, which is why I intentionally left attach/detach based on VID/PID as is. Handling this case properly introduces additional complexity, such as carrying crosvm state, dealing with mutability, handling cases where a single device matches the rules of multiple VMs, etc.

If we do want to merge this, I would at least simplify the implementation:

• There's no need to modify vmm.py and device.py by adding crosvm_usb_port there, since this belongs to a different abstraction layer. Crosvm-specific data should remain isolated in its own class.
• In crosvm.py, there's no need to introduce new exception classes or change the parsing logic, other than fixing existing parsing errors if necessary.
• There's no need to modify the state database to handle socket generation, etc. For example, Ghaf uses a proxy for VMM sockets so we might not have direct access to the crosvm socket anyway.
• The workaround with no_available_port that the AI removed was something I had to add because, in my testing, USB hotplugging sometimes stopped working, maybe after 1 out of 10 reboots. In crosvm, USB hotplugging can only be done after the guest kernel has booted, but we don't have a reliable way to determine when that has happened. If we remove this workaround, we need to be sure that this problem no longer exists.