diff --git a/.github/scripts/tests/test_usbip.py b/.github/scripts/tests/test_usbip.py index 51bda879..69bd1fc5 100644 --- a/.github/scripts/tests/test_usbip.py +++ b/.github/scripts/tests/test_usbip.py @@ -40,7 +40,18 @@ class USBIPTests(unittest.TestCase): ("SERVER_RUN_DIR", "server-run"), ("UNIT_DIR", "units"), ("SYSFS", "sysfs")): setattr(self.m, attr, self.root / path) getattr(self.m, attr).mkdir() - self.m.MATCH_BUSID = self.root / "match_busid" + class MatchTable: + # Mirror the kernel contract, not regular-file write semantics: + # del(ALLOC)=REMOV, del(ADDED/REMOV) retains the named slot. + def __init__(self): + self.slots = {} + def write_text(self, value): + action, busid = value.split() + if self.slots.get(busid) == "ALLOC": + self.slots[busid] = "REMOV" + def read_text(self): + return " ".join(self.slots) + self.m.MATCH_BUSID = MatchTable() self.account = SimpleNamespace(pw_uid=1234, pw_gid=1234) self.calls = [] self.key = self.root / "id_ed25519" @@ -80,8 +91,10 @@ class USBIPTests(unittest.TestCase): self.calls.append(args) if args[:2] == ["usbip", "bind"]: self.set_driver(args[-1], "usbip-host") + self.m.MATCH_BUSID.slots[args[-1]] = "ALLOC" elif args[:2] == ["usbip", "unbind"]: self.set_driver(args[-1], "usb") + self.m.MATCH_BUSID.slots.pop(args[-1], None) return "" def test_storage_composite_interfaces_and_hubs_are_never_exported(self): @@ -109,7 +122,7 @@ class USBIPTests(unittest.TestCase): self.assertEqual(path.stat().st_mode & 0o777, 0o600) self.assertNotEqual(self.m.RUN_DIR, self.m.SERVER_RUN_DIR) - def test_serial_moves_to_another_port_without_unbinding_a_replacement(self): + def test_port_move_fails_closed_when_kernel_retains_old_slot(self): selected = self.selected(self.device()) config = self.config([selected]) owned = {} @@ -118,10 +131,11 @@ class USBIPTests(unittest.TestCase): shutil.rmtree(self.m.SYSFS / "1-2") self.device(serial="other-device", driver="usbip-host") self.device(busid="2-4") - self.m.reconcile_exports(config, owned) - self.assertEqual(owned[EXPORT]["busid"], "2-4") - self.assertEqual(self.calls, [["usbip", "bind", "--busid", "1-2"], ["usbip", "bind", "--busid", "2-4"]]) - self.assertEqual(self.m.catalog()[0]["busid"], "2-4") + errors = self.m.reconcile_exports(config, owned) + self.assertIn("blocked", errors[EXPORT]) + self.assertEqual(owned[EXPORT]["busid"], "1-2") + self.assertEqual(self.calls, [["usbip", "bind", "--busid", "1-2"]]) + self.assertIsNone(self.m.catalog()[0]["busid"]) def test_no_serial_requires_same_busid_and_ambiguous_serial_is_rejected(self): selected = self.selected(self.device(serial="")) @@ -170,7 +184,7 @@ class USBIPTests(unittest.TestCase): self.assertEqual(self.m.status()["server_status"]["errors"], {EXPORT: "bind failed"}) self.assertEqual(self.m.catalog()[0]["status"], "waiting") - def test_unplug_clears_owned_stale_match_before_waiting(self): + def test_unplugged_kernel_match_is_disarmed_but_not_falsely_forgotten(self): selected = self.selected(self.device()) config = self.config([selected]) owned = {} @@ -178,9 +192,9 @@ class USBIPTests(unittest.TestCase): self.m.reconcile_exports(config, owned) shutil.rmtree(self.m.SYSFS / "1-2") errors = self.m.reconcile_exports(config, owned) - self.assertEqual(owned, {}) - self.assertIn(EXPORT, errors) - self.assertEqual(self.m.MATCH_BUSID.read_text(), "del 1-2") + self.assertEqual(owned[EXPORT]["busid"], "1-2") + self.assertIn("blocked", errors[EXPORT]) + self.assertEqual(self.m.MATCH_BUSID.slots["1-2"], "REMOV") self.assertEqual(len(self.calls), 1) def test_failed_stale_cleanup_does_not_forget_or_replace_ownership(self): @@ -189,9 +203,9 @@ class USBIPTests(unittest.TestCase): owned = {EXPORT: selected} shutil.rmtree(self.m.SYSFS / "1-2") self.device(busid="2-4") - with patch.object(Path, "write_text", side_effect=PermissionError), patch.object(self.m, "run") as command: + with patch.object(self.m, "disarm_match", side_effect=self.m.USBIPError("disarm failed")), patch.object(self.m, "run") as command: errors = self.m.reconcile_exports(config, owned) - self.assertIn("stale", errors[EXPORT]) + self.assertIn("disarm", errors[EXPORT]) self.assertEqual(owned[EXPORT]["busid"], "1-2") command.assert_not_called() diff --git a/docs/usbip.md b/docs/usbip.md index c5b37db9..32255d80 100644 --- a/docs/usbip.md +++ b/docs/usbip.md @@ -22,11 +22,15 @@ requested in [#350](https://github.com/MacRimi/ProxMenux/issues/350). journalctl -u proxmenux-usbip-server.service ``` -The server reloads saved selections every three seconds. After unplug or a port -change it removes the recorded stale `match_busid` entry even when the original -device is absent; a replacement device is never explicitly unbound. This is -polling-based detection, not an atomic guarantee against very fast port reuse. Devices with a unique -serial number can return on a different physical USB port after reconnecting. +The server reloads saved selections every three seconds. A live exported device's +match is immediately marked for removal so the kernel rejects a later automatic +probe on that port. Physical unplug/replug remains an **unresolved limitation**: +upstream Linux can retain the match-table slot even after a successful `del` +write. The manager verifies disappearance before forgetting ownership or binding +a new port, and otherwise fails closed with a visible cleanup error. It never +calls `rebind` on an absent device or reloads a module affecting foreign exports. +Automatic reconnect after physical USB removal is therefore not yet guaranteed; +this PR needs maintainer review and a safe kernel-supported cleanup policy. Without a serial number, the vendor/product and original bus ID must match; the manager will not guess which identical dongle to bind. Duplicate serial numbers are rejected as ambiguous. USB hubs and mass-storage devices, including @@ -120,7 +124,7 @@ python3 "$MANAGER" status The client has an enabled systemd service per device. It reconnects the SSH tunnel and reattaches the device after a network interruption or restart. On each new attach it resolves the **saved export ID** to the server's current bus -ID, so a serial-bearing dongle can move ports. The client keeps an ownership +ID; physical port movement is subject to the kernel cleanup limitation above. The client keeps an ownership record of the exact imported kernel port, local bus ID and remote endpoint; cleanup detaches only a matching owned import. Removing a connection disables its service, releases its device and removes its saved configuration. diff --git a/scripts/devices/usbip_manager.py b/scripts/devices/usbip_manager.py index 042837b3..3e36dcb1 100644 --- a/scripts/devices/usbip_manager.py +++ b/scripts/devices/usbip_manager.py @@ -162,6 +162,16 @@ def state_path(): return SERVER_RUN_DIR / "exports.json" +def disarm_match(busid): + try: + MATCH_BUSID.write_text(f"del {busid}") + except FileNotFoundError: + pass + except OSError as exc: + if exc.errno != errno.ENODEV: + raise USBIPError("Could not disarm the USB/IP match") from exc + + def release_export(identifier, owned, devices): previous = owned[identifier] current = next((d for d in devices if d["busid"] == previous["busid"]), None) @@ -170,15 +180,16 @@ def release_export(identifier, owned, devices): if current and same_device(previous, current) and current["driver"] == "usbip-host": run(["usbip", "unbind", "--busid", current["busid"]]) else: - # usbip unbind requires a present device. Delete our stale match even - # after unplug, without unbinding a replacement or a foreign export. + # A successful 'del' write does not prove removal on every kernel: + # upstream retains ADDED/REMOV table slots after physical unplug. + # Never forget ownership or bind a new port while that slot survives. + disarm_match(previous["busid"]) try: - MATCH_BUSID.write_text(f"del {previous['busid']}") + remaining = MATCH_BUSID.read_text().split() except FileNotFoundError: - pass # An unloaded driver has no persistent match table. - except OSError as exc: - if exc.errno != errno.ENODEV: - raise USBIPError("Could not remove the stale USB/IP match") from exc + remaining = [] + if previous["busid"] in remaining: + raise USBIPError("Kernel retained the unplugged USB/IP match; automatic reconnect is blocked pending safe cleanup") del owned[identifier] atomic_json(state_path(), owned) @@ -207,11 +218,15 @@ def reconcile_exports(config, owned): if device["driver"] == "usbip-host": if not previous: raise USBIPError("Device is already exported outside ProxMenux; it was not adopted") + disarm_match(device["busid"]) continue # Record intent before binding, so a crash cannot orphan our export. owned[identifier] = {k: device[k] for k in ("busid", "vendor", "product", "serial")} atomic_json(state_path(), owned) run(["usbip", "bind", "--busid", device["busid"]]) + # ALLOC -> REMOV rejects future probes without interrupting this + # live export. Do not leave a reusable automatic port match armed. + disarm_match(device["busid"]) except USBIPError as exc: errors[identifier] = str(exc) atomic_json(SERVER_RUN_DIR / "status.json", {"errors": errors, "updated": time.time()})