diff --git a/docs/README.md b/docs/README.md index 79908e27c..c347479cc 100644 --- a/docs/README.md +++ b/docs/README.md @@ -109,7 +109,7 @@ Cisco XRd as a GNS3 Docker router: vendor path + shm/device injection (`GNS3_SHM ## Known Issues (`bugs/`) - [Telnet Server Connection Race Condition](bugs/telnet-server-connection-race-condition.md) — `getpeername()` error when client disconnects during connection setup (High severity, Open) -- [Docker Link UDP Self-Loop](bugs/link-udp-self-loop.md) — intermittent one-way link (one end's NIO points at itself); delete/recreate the link as workaround (Medium severity, Open) +- [Docker Link UDP Self-Loop](bugs/link-udp-self-loop.md) — intermittent one-way link (both ends handed the same UDP port by an allocation race); **fixed** (Medium severity) --- diff --git a/docs/bugs/link-udp-self-loop.md b/docs/bugs/link-udp-self-loop.md index 98c4ba1cc..e1e05e7cf 100644 --- a/docs/bugs/link-udp-self-loop.md +++ b/docs/bugs/link-udp-self-loop.md @@ -5,25 +5,24 @@ See LICENSE file for licensing information. > This documentation is organized by AI with reference to actual code. AI can make mistakes — please verify against the source code when in doubt. - # Docker Link UDP Self-Loop Bug (One-Way Link) ## Bug Report **Date**: 2026-08-14 **Severity**: Medium (one-way connectivity, CPU burn from packet duplication; intermittent) -**Status**: Open — root cause not yet pinpointed; workaround reliable -**Component**: Link wiring — `gns3server/controller/udp_link.py` (`_prepare` / -`pop_preallocated_udp_port` in `gns3server/controller/project.py`) interacting with -node/uBridge restarts +**Status**: **Fixed** — root cause found and unit-tested (same day) +**Component**: UDP port allocation — `gns3server/compute/port_manager.py` +(`get_free_udp_port` find-then-add race); secondary: `gns3server/controller/udp_link.py` +(`_prepare` accumulated stale `_link_data` on reset) ## Symptoms -Two Docker nodes (observed with Cisco XRd; likely node-type agnostic) linked on the +Two Docker nodes (observed with Cisco XRd; node-type agnostic) linked on the same compute cannot ping each other. Packet capture on the link shows only **one** side sending ARP. The other side's traffic never appears on the link at all. -## Evidence (from a live occurrence) +## Evidence (from the live occurrence) Captured simultaneously on both nodes' uBridge `bridge1` (see diagnostics below): @@ -34,23 +33,44 @@ Captured simultaneously on both nodes' uBridge `bridge1` (see diagnostics below) | B's `ethN` counters | RX ≈ TX ≈ 5000+ — B receives its own transmissions back | | A's `ethN` counters | TX > 0, RX = 0 — never receives anything | -Conclusion: **B's `nio_udp` rport pointed at B's own lport** — a UDP self-loop. B's -uBridge had restarted ~77 s after A's (node stop/start during the session), -i.e. the link was re-wired at least once. +## Root Cause (confirmed) -Deleting and re-creating the link fixed it immediately (fresh port allocation). +**`PortManager.get_free_udp_port` had an unguarded find-then-add sequence.** +A link allocates the UDP port for **both ends concurrently** +(`asyncio.gather` in `UDPLink._prepare` → two `POST /ports/udp`). The route +handler is a sync `def`, so FastAPI executes the two requests **in parallel +threads**. Both threads ran `find_unused_port` (socket-probing, GIL-releasing) +before either reached `_used_udp_ports.add(port)` — the set add is idempotent, +so no error was raised and **both ends were handed the same port number**: +`lport == rport` on both NIOs, a literal self-loop. -## Root Cause Analysis (narrowed, not final) +Why it was *silent* and *asymmetric*: -- `UDPLink._prepare()` builds mirrored NIO data correctly - (`node1: lport=P1/rport=P2`, `node2: lport=P2/rport=P1`) — the logic itself is sound. -- Suspects for the corrupted runtime state: - 1. `Project.pop_preallocated_udp_port()` — the batch project-open preallocation - (link-create performance work) racing with link re-creation; - 2. link re-creation racing a node/uBridge restart (commit NIOs to a uBridge that is - being torn down/rebuilt), leaving a stale/self-pointing NIO on one side. -- A deterministic reproduction is still needed: create two Docker nodes + link, - restart one node, then verify the UDP wiring (see diagnostics). +- uBridge sets `SO_REUSEADDR` on UDP NIO sockets (`ubridge/src/nio_udp.c`), so + the second bind of the same port **succeeds** instead of failing with + `EADDRINUSE` — link creation returned success. +- With two sockets bound to the same port, the kernel delivers to one of them + (last bound wins). The node that started later — in the live case B, + restarted ~77 s after A — received **everything**: A's packets *and* its own + transmissions echoed back. The 77 s restart did not cause the corruption; it + only decided which end starves. +- The same-compute condition is part of the trigger: both allocations hit the + same `PortManager` instance (a cross-compute link races two processes and + cannot self-collide). + +A second, smaller defect was found while auditing: `UDPLink._prepare()` +**appended** to `self._link_data` but the committed NIOs are always taken from +indices 0/1 — after `reset()` (delete + create on the same object) the stale, +already-released port pair was re-committed and the freshly allocated ports +were leaked. + +## Fix + +| Change | Where | +|---|---| +| `threading.RLock` making find-then-add (and reserve/release) atomic for TCP and UDP | `gns3server/compute/port_manager.py` | +| `_prepare()` rebuilds `_link_data` from scratch instead of appending | `gns3server/controller/udp_link.py` | +| Regression tests: threaded allocation never returns duplicates (red on the old code); `reset()` commits the fresh mirrored pair with `lport != rport` | `tests/compute/test_port_manager.py`, `tests/controller/test_udp_link.py` | ## Diagnostics (uBridge console is the fast path) @@ -63,8 +83,10 @@ Deleting and re-creating the link fixed it immediately (fresh port allocation). 2. **Container counters** — `docker exec ip -s link show ethN`: TX>0/RX=0 → peer never returns; RX≈TX huge with µs-scale duplicates → self-loop. 3. **UDP sockets** — `ss -uln` (no `-p`; uBridge runs setuid-root so process names - are hidden, ports are visible): a two-node link owns exactly two "orphan" UDP ports. -4. **Workaround** — delete and re-create the link (or stop/start both nodes). + are hidden, ports are visible): a two-node link owns exactly two "orphan" UDP + ports. **One port instead of two = this bug.** +4. **Recovery** — delete and re-create the link (or stop/start both nodes); + with the fix, the corruption no longer occurs in the first place. Note: a Docker node's in-container `ethN` is a **TAP device** whose file descriptor lives inside uBridge (the interface is created host-side, then moved into the @@ -74,4 +96,6 @@ waste time hunting for one in the host namespace. ## Related - `docs/features/vendor-nos-xrd.md` — troubleshooting table entry pointing here. -- Link-create batch optimization (`controller/udp_link.py`, project-open bulk path). +- Link-create batch optimization (`controller/udp_link.py`, project-open bulk path) + was exonerated: the pool path allocates sequentially in one handler and cannot + self-collide. diff --git a/docs/features/vendor-nos-xrd.md b/docs/features/vendor-nos-xrd.md index 633a4b580..68b7a21e3 100644 --- a/docs/features/vendor-nos-xrd.md +++ b/docs/features/vendor-nos-xrd.md @@ -158,7 +158,7 @@ sequenceDiagram | `Invalid interface entries ... XR_MGMT_INTERFACES` | use `xr_name=Mg0/RP0/CPU0/0` | | Host audio muted / USB reconnects when the node starts | set `GNS3_MASK_UDEV=1` | | Config lost across stop/start | `extra_volumes` must be `/xr-storage-shadow` | -| Two nodes can't ping, only one side ARPs | GNS3 link wiring bug (UDP self-loop on one end), not XRd — delete and re-create the link; see [link-udp-self-loop](../bugs/link-udp-self-loop.md) | +| Two nodes can't ping, only one side ARPs | GNS3 link wiring bug (UDP self-loop on one end), not XRd — fixed (port-allocation race); on older builds delete and re-create the link; see [link-udp-self-loop](../bugs/link-udp-self-loop.md) | | Compute log: busybox coredump storm | fixed by the container-chown change; verify gns3-server is current | ## Notes @@ -191,5 +191,6 @@ sequenceDiagram | Version | Date | Changes | |---------|------|---------| +| 1.2 | 2026-08-14 | Link UDP self-loop root-caused to a port-allocation race and fixed — see [link-udp-self-loop](../bugs/link-udp-self-loop.md) (now marked Fixed). | | 1.1 | 2026-08-14 | Datapath validated end-to-end (XRd brings its own interfaces up; ARP/ICMP bidirectional). Add troubleshooting entry for the one-way-link symptom (GNS3 link UDP self-loop bug, see bugs/link-udp-self-loop.md). | | 1.0 | 2026-08-14 | Initial documentation of the XRd control-plane adaptation: vendor path requirement, shm/devices/extra_configs/udev-mask mechanisms, host-disturbance root causes, appliance recipe. | diff --git a/gns3server/compute/port_manager.py b/gns3server/compute/port_manager.py index 6dd2549a8..dbb18fc0e 100644 --- a/gns3server/compute/port_manager.py +++ b/gns3server/compute/port_manager.py @@ -16,6 +16,7 @@ import socket import ipaddress +import threading from fastapi import HTTPException, status from gns3server.config import Config @@ -105,6 +106,13 @@ class PortManager: self._udp_host = "0.0.0.0" self._used_tcp_ports = set() self._used_udp_ports = set() + # Guards the find-then-add port allocation against concurrent threads: + # FastAPI runs sync route handlers (e.g. POST /ports/udp) in a thread + # pool, and a link allocates both of its ends concurrently — without + # the lock both threads can probe the same "free" port and hand the + # same number to both ends of a link (lport == rport self-loop). + # RLock because reserve_*_port falls back to get_free_*_port. + self._lock = threading.RLock() console_start_port_range = Config.instance().settings.Server.console_start_port_range console_end_port_range = Config.instance().settings.Server.console_end_port_range @@ -275,16 +283,17 @@ class PortManager: port_range_start = self._console_port_range[0] port_range_end = self._console_port_range[1] - port = self.find_unused_port( - port_range_start, - port_range_end, - host=self._console_host, - socket_type="TCP", - ignore_ports=self._used_tcp_ports, - ) + with self._lock: + port = self.find_unused_port( + port_range_start, + port_range_end, + host=self._console_host, + socket_type="TCP", + ignore_ports=self._used_tcp_ports, + ) - self._used_tcp_ports.add(port) - project.record_tcp_port(port) + self._used_tcp_ports.add(port) + project.record_tcp_port(port) log.debug(f"TCP port {port} has been allocated") return port @@ -305,32 +314,33 @@ class PortManager: port_range_start = self._console_port_range[0] port_range_end = self._console_port_range[1] - if port in self._used_tcp_ports: - old_port = port - port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end) - msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}" - log.debug(msg) - return port - if port < port_range_start or port > port_range_end: - old_port = port - port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end) - msg = ( - f"TCP port {old_port} is outside the range {port_range_start}-{port_range_end} on host " - f"{self._console_host}. Port has been replaced by {port}" - ) - log.debug(msg) - return port - try: - PortManager._check_port(self._console_host, port, "TCP") - except OSError: - old_port = port - port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end) - msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}" - log.debug(msg) - return port + with self._lock: + if port in self._used_tcp_ports: + old_port = port + port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end) + msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}" + log.debug(msg) + return port + if port < port_range_start or port > port_range_end: + old_port = port + port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end) + msg = ( + f"TCP port {old_port} is outside the range {port_range_start}-{port_range_end} on host " + f"{self._console_host}. Port has been replaced by {port}" + ) + log.debug(msg) + return port + try: + PortManager._check_port(self._console_host, port, "TCP") + except OSError: + old_port = port + port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end) + msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}" + log.debug(msg) + return port - self._used_tcp_ports.add(port) - project.record_tcp_port(port) + self._used_tcp_ports.add(port) + project.record_tcp_port(port) log.debug(f"TCP port {port} has been reserved") return port @@ -342,10 +352,11 @@ class PortManager: :param project: Project instance """ - if port in self._used_tcp_ports: - self._used_tcp_ports.remove(port) - project.remove_tcp_port(port) - log.debug(f"TCP port {port} has been released") + with self._lock: + if port in self._used_tcp_ports: + self._used_tcp_ports.remove(port) + project.remove_tcp_port(port) + log.debug(f"TCP port {port} has been released") def get_free_udp_port(self, project): """ @@ -353,16 +364,17 @@ class PortManager: :param project: Project instance """ - port = self.find_unused_port( - self._udp_port_range[0], - self._udp_port_range[1], - host=self._udp_host, - socket_type="UDP", - ignore_ports=self._used_udp_ports, - ) + with self._lock: + port = self.find_unused_port( + self._udp_port_range[0], + self._udp_port_range[1], + host=self._udp_host, + socket_type="UDP", + ignore_ports=self._used_udp_ports, + ) - self._used_udp_ports.add(port) - project.record_udp_port(port) + self._used_udp_ports.add(port) + project.record_udp_port(port) log.debug(f"UDP port {port} has been allocated") return port @@ -374,18 +386,20 @@ class PortManager: :param project: Project instance """ - if port in self._used_udp_ports: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=f"UDP port {port} already in use on host {self._console_host}", - ) - if port < self._udp_port_range[0] or port > self._udp_port_range[1]: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=f"UDP port {port} is outside the range " f"{self._udp_port_range[0]}-{self._udp_port_range[1]}", - ) - self._used_udp_ports.add(port) - project.record_udp_port(port) + with self._lock: + if port in self._used_udp_ports: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"UDP port {port} already in use on host {self._console_host}", + ) + if port < self._udp_port_range[0] or port > self._udp_port_range[1]: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"UDP port {port} is outside the range " + f"{self._udp_port_range[0]}-{self._udp_port_range[1]}", + ) + self._used_udp_ports.add(port) + project.record_udp_port(port) log.debug(f"UDP port {port} has been reserved") def release_udp_port(self, port, project): @@ -396,7 +410,8 @@ class PortManager: :param project: Project instance """ - if port in self._used_udp_ports: - self._used_udp_ports.remove(port) - project.remove_udp_port(port) - log.debug(f"UDP port {port} has been released") + with self._lock: + if port in self._used_udp_ports: + self._used_udp_ports.remove(port) + project.remove_udp_port(port) + log.debug(f"UDP port {port} has been released") diff --git a/gns3server/controller/udp_link.py b/gns3server/controller/udp_link.py index 86f863010..1184dad74 100644 --- a/gns3server/controller/udp_link.py +++ b/gns3server/controller/udp_link.py @@ -98,6 +98,13 @@ class UDPLink(Link): tuples, ready to be POSTed to each node's compute. """ + # Start from a clean slate: reset() re-creates the link on the same + # object (delete() + create()), and _commit_nios()/update() always + # address indices 0/1 — appending onto the previous run's entries + # would re-commit the stale (already released) port pair and orphan + # the freshly allocated ports. + self._link_data = [] + node1 = self._nodes[0]["node"] adapter_number1 = self._nodes[0]["adapter_number"] port_number1 = self._nodes[0]["port_number"] diff --git a/tests/compute/test_port_manager.py b/tests/compute/test_port_manager.py index b12af3206..ede2cea7a 100644 --- a/tests/compute/test_port_manager.py +++ b/tests/compute/test_port_manager.py @@ -16,6 +16,7 @@ # along with this program. If not, see . import pytest +import threading import uuid from fastapi import HTTPException @@ -116,6 +117,82 @@ def test_release_udp_port(): pm.reserve_udp_port(20000, project) +def test_concurrent_udp_port_allocation_no_duplicates(): + """ + Regression test for the link UDP self-loop bug (docs/bugs/link-udp-self-loop.md): + both ends of a link are allocated concurrently on the controller + (asyncio.gather -> two POST /ports/udp), and FastAPI runs the sync route + handler in a threadpool. The find-then-add allocation must be atomic, + otherwise both threads can probe and return the same "free" port — + handing lport == rport to both ends, which makes every packet loop back + to its sender (one-way link). + """ + + pm = PortManager() + pm.udp_port_range = (50000, 50100) + project = Project(project_id=str(uuid.uuid4())) + + workers = 8 + rounds = 10 + barrier = threading.Barrier(workers) + results = [] + results_lock = threading.Lock() + + def worker(): + allocated = [] + for _ in range(rounds): + # start each round together to maximize the collision window + barrier.wait() + allocated.append(pm.get_free_udp_port(project)) + with results_lock: + results.extend(allocated) + + threads = [threading.Thread(target=worker) for _ in range(workers)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert len(results) == workers * rounds + assert len(set(results)) == len(results), "the same UDP port was handed to two callers" + assert pm.udp_ports == set(results) + + +def test_concurrent_tcp_port_allocation_no_duplicates(): + """ + Same race class as the UDP self-loop bug, on the console/TCP side: + concurrent get_free_tcp_port calls must never return the same port. + """ + + pm = PortManager() + pm.console_port_range = (51000, 51100) + project = Project(project_id=str(uuid.uuid4())) + + workers = 8 + rounds = 10 + barrier = threading.Barrier(workers) + results = [] + results_lock = threading.Lock() + + def worker(): + allocated = [] + for _ in range(rounds): + barrier.wait() + allocated.append(pm.get_free_tcp_port(project)) + with results_lock: + results.extend(allocated) + + threads = [threading.Thread(target=worker) for _ in range(workers)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert len(results) == workers * rounds + assert len(set(results)) == len(results), "the same TCP port was handed to two callers" + assert pm.tcp_ports == set(results) + + def test_find_unused_port(): p = PortManager().find_unused_port(1000, 10000) diff --git a/tests/controller/test_udp_link.py b/tests/controller/test_udp_link.py index e322627a0..b17120757 100644 --- a/tests/controller/test_udp_link.py +++ b/tests/controller/test_udp_link.py @@ -187,6 +187,78 @@ async def test_delete(project): compute2.delete.assert_any_call("/projects/{}/vpcs/nodes/{}/adapters/3/ports/1/nio".format(project.id, node2.id), timeout=120) +@pytest.mark.asyncio +async def test_reset(project): + """ + reset() re-creates the link on the same object: the fresh port pair must + replace the stale one instead of accumulating (the committed NIOs always + come from indices 0/1) — see docs/bugs/link-udp-self-loop.md. + """ + + compute1 = MagicMock() + compute2 = MagicMock() + + node1 = Node(project, compute1, "node1", node_type="vpcs") + node1._ports = [EthernetPort("E0", 0, 0, 4)] + node2 = Node(project, compute2, "node2", node_type="vpcs") + node2._ports = [EthernetPort("E0", 0, 3, 1)] + + async def subnet_callback(compute2): + """ + Fake subnet callback + """ + return ("192.168.1.1", "192.168.1.2") + + compute1.get_ip_on_same_subnet.side_effect = subnet_callback + + # per-compute port sequences: first create -> 1024/2048, reset -> 4096/8192 + node1_ports = iter([1024, 4096]) + node2_ports = iter([2048, 8192]) + + async def compute1_callback(path, data={}, **kwargs): + if "/ports/udp" in path: + response = MagicMock() + response.json = {"udp_port": next(node1_ports)} + return response + + async def compute2_callback(path, data={}, **kwargs): + if "/ports/udp" in path: + response = MagicMock() + response.json = {"udp_port": next(node2_ports)} + return response + + compute1.post.side_effect = compute1_callback + compute1.host = "example.com" + compute2.post.side_effect = compute2_callback + compute2.host = "example.org" + + link = UDPLink(project) + await link.add_node(node1, 0, 4) + await link.add_node(node2, 3, 1) + + await link.reset() + + # exactly one (fresh) NIO spec per side — no stale entries left behind + assert len(link.debug_link_data) == 2 + assert link.debug_link_data[0]["lport"] == 4096 + assert link.debug_link_data[0]["rport"] == 8192 + assert link.debug_link_data[1]["lport"] == 8192 + assert link.debug_link_data[1]["rport"] == 4096 + # the self-loop invariant: an end's lport must never equal its rport + assert link.debug_link_data[0]["lport"] != link.debug_link_data[0]["rport"] + assert link.debug_link_data[1]["lport"] != link.debug_link_data[1]["rport"] + # the committed NIO carries the fresh pair, not the released one + compute1.post.assert_any_call("/projects/{}/vpcs/nodes/{}/adapters/0/ports/4/nio".format(project.id, node1.id), data={ + "lport": 4096, + "rhost": "192.168.1.2", + "rport": 8192, + "type": "nio_udp", + "filters": {}, + "markers": {}, + "suspend": False, + }, timeout=120) + + @pytest.mark.asyncio async def test_choose_capture_side(project): """