Fail closed when the kernel retains an unplugged USB/IP match

.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()
 

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.

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()})