diff --git a/docs/features/docker-kernel-datapath.md b/docs/features/docker-kernel-datapath.md index 8e36cd1ce..d2f7232e1 100644 --- a/docs/features/docker-kernel-datapath.md +++ b/docs/features/docker-kernel-datapath.md @@ -243,7 +243,7 @@ The veth pairs already exist at attach time — every adapter is born as a veth ## Capture -Kernel links capture via uBridge's AF_PACKET module bound to the veth host end: `capture start_kernel "" [dlt]` / `capture stop_kernel`. Single capture per uBridge process (second concurrent → EALREADY). Start/stop key on the **NIO type**, not on veth presence — a relay NIO riding a veth captures at its relay bridge. +Kernel links capture via uBridge's AF_PACKET module bound to the veth host end: `capture start_kernel "" [dlt]` / `capture stop_kernel`. Single capture per uBridge process (second concurrent → EALREADY): the server tracks which anchor owns the slot, refuses a second port's capture with a clear error instead of leaving it wedged, and only the owning port's stop issues the process-wide `capture stop_kernel`. Start/stop key on the **NIO type**, not on veth presence — a relay NIO riding a veth captures at its relay bridge. ## Markers diff --git a/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py b/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py index 4bf507b25..5d2506a65 100644 --- a/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py +++ b/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py @@ -200,7 +200,11 @@ class ExecuteMultipleDeviceConfigCommands(BaseTool): return device_configs_list # Merge same-device entries before anything keys by device_name - device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="config_commands") + try: + device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="config_commands") + except ValueError as e: + logger.error("Invalid device_configs input: %s", e) + return [{"status": "failed", "error": str(e)}] # Filter forbidden commands and store blocked commands info device_configs_list, blocked_commands_map = self._filter_forbidden_commands_from_device_configs( diff --git a/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py b/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py index afdeac035..bbb92b0c5 100644 --- a/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py +++ b/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py @@ -203,7 +203,11 @@ class ExecuteMultipleDeviceCommands(BaseTool): return device_configs_list # Merge same-device entries before anything keys by device_name - device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="commands") + try: + device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="commands") + except ValueError as e: + logger.error("Invalid device_configs input: %s", e) + return [{"status": "failed", "error": str(e)}] # Filter forbidden commands and store blocked commands info device_configs_list, blocked_commands_map = self._filter_forbidden_commands_from_device_configs( diff --git a/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py b/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py index 31ed0c916..ed33642ef 100644 --- a/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py +++ b/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py @@ -183,7 +183,11 @@ class VPCSCommands(BaseTool): return device_configs_list # Merge same-device entries before anything keys by device_name - device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="commands") + try: + device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="commands") + except ValueError as e: + logger.error("Invalid device_configs input: %s", e) + return [{"status": "failed", "error": str(e)}] # Create a mapping of device names to their commands device_configs_map = self._configs_map(device_configs_list) diff --git a/gns3server/agent/gns3_copilot/utils/device_configs.py b/gns3server/agent/gns3_copilot/utils/device_configs.py index e757eb089..f5ff4575f 100644 --- a/gns3server/agent/gns3_copilot/utils/device_configs.py +++ b/gns3server/agent/gns3_copilot/utils/device_configs.py @@ -34,7 +34,7 @@ from typing import Any def merge_duplicate_device_configs( - device_configs: list[dict[str, Any]], + device_configs: list[Any], commands_field: str, ) -> list[dict[str, Any]]: """ @@ -46,8 +46,9 @@ def merge_duplicate_device_configs( every row would report that same output. Args: - device_configs: Device config entries; each holds device_name and a - command list under commands_field. + device_configs: Device config entries (parsed tool input, validated + here); each holds device_name and a command list under + commands_field. commands_field: Field carrying the commands ("commands" or "config_commands" depending on the tool). @@ -55,12 +56,18 @@ def merge_duplicate_device_configs( One entry per device_name, in first-appearance order. The first entry's extra fields are preserved; later duplicates only contribute their commands. + + Raises: + ValueError: An entry is not an object or has no usable device_name. + Such an entry cannot be addressed; dropping it silently would + let a malformed batch execute partially while every returned + row reports success. """ merged: dict[str, dict[str, Any]] = {} - for config in device_configs: - name = config.get("device_name") - if not name: - continue + for index, config in enumerate(device_configs): + if not isinstance(config, dict) or not config.get("device_name"): + raise ValueError(f"device_configs[{index}] must be an object with a non-empty 'device_name' field") + name = config["device_name"] commands = list(config.get(commands_field, [])) if name in merged: merged[name][commands_field].extend(commands) diff --git a/gns3server/api/routes/compute/ethernet_switch_nodes.py b/gns3server/api/routes/compute/ethernet_switch_nodes.py index ba10182d8..82546f118 100644 --- a/gns3server/api/routes/compute/ethernet_switch_nodes.py +++ b/gns3server/api/routes/compute/ethernet_switch_nodes.py @@ -114,7 +114,16 @@ async def update_ethernet_switch( # place (the setter itself keeps the port-count guard) previous_mapping = [dict(port) for port in node.ports_mapping] node.ports_mapping = data["ports_mapping"] - await node.update_port_settings(previous_mapping) + try: + await node.update_port_settings(previous_mapping) + except Exception: + # Nothing, or only part, of the new mapping was applied — keep + # the old one so the failed change stays visible in GET and a + # retry still diffs against it. A kept-new mapping would turn + # the retry into a no-op that reports success with the VLANs + # never applied. + node.ports_mapping = previous_mapping + raise if "console_type" in data: node.console_type = data["console_type"] node.updated() diff --git a/gns3server/compute/base_node.py b/gns3server/compute/base_node.py index 7ff62ae07..6a85a97c7 100644 --- a/gns3server/compute/base_node.py +++ b/gns3server/compute/base_node.py @@ -1065,6 +1065,10 @@ class BaseNode: # than skipping them as "already installed". self._marker_filter_bridges.clear() self._marker_specs.clear() + # ...and so is the kernel capture, uBridge's single AF_PACKET slot + # (meaningful for the kernel-datapath node types; see + # KernelDatapathMixin._reserve_kernel_capture). + self._kernel_capture_ifc = None async def add_ubridge_udp_connection(self, bridge_name, source_nio, destination_nio): """ @@ -1116,9 +1120,18 @@ class BaseNode: :param filters: Array of filter dictionary """ - # The netem-extension filters (rate, reorder, gemodel…) only exist on - # the kernel datapath. The controller keeps them off relay links; - # this is the second guard for direct compute API use. + self._guard_kernel_only_filters(filters) + await self._ubridge_send("bridge reset_packet_filters " + bridge_name) + for packet_filter in self._build_filter_list(filters): + await self._ubridge_add_packet_filter(f"bridge add_packet_filter {bridge_name} {packet_filter}") + + def _guard_kernel_only_filters(self, filters): + """ + The netem-extension filters (rate, reorder, gemodel…) only exist on + the kernel datapath. The controller keeps them off relay links; this + is the second guard for direct compute API use. + """ + from gns3server.utils.packet_filter_validation import kernel_only_features kernel_only = kernel_only_features(filters) @@ -1127,19 +1140,25 @@ class BaseNode: "Packet filter(s) {} only run on a kernel-datapath link (tc netem on the " "veth host end); the uBridge relay has no equivalent".format(", ".join(sorted(kernel_only))) ) - await self._ubridge_send("bridge reset_packet_filters " + bridge_name) - for packet_filter in self._build_filter_list(filters): - cmd = f"bridge add_packet_filter {bridge_name} {packet_filter}" - try: - await self._ubridge_send(cmd) - except UbridgeError as e: - match = re.search(r"Cannot compile filter '(.*)': syntax error", str(e)) - if match: - message = f"Warning: ignoring BPF packet filter '{self.name}' due to syntax error: {match.group(1)}" - log.warning(message) - self.project.emit("log.warning", {"message": message}) - else: - raise + + async def _ubridge_add_packet_filter(self, cmd): + """ + Send one relay filter command, degrading a BPF expression that no + longer compiles on this uBridge's libpcap to a warning instead of + breaking the link (the controller already validated the syntax with + tcpdump at create/update time). + """ + + try: + await self._ubridge_send(cmd) + except UbridgeError as e: + match = re.search(r"Cannot compile filter '(.*)': syntax error", str(e)) + if match: + message = f"Warning: ignoring BPF packet filter '{self.name}' due to syntax error: {match.group(1)}" + log.warning(message) + self.project.emit("log.warning", {"message": message}) + else: + raise def _build_filter_list(self, filters): """ diff --git a/gns3server/compute/builtin/nodes/ethernet_switch.py b/gns3server/compute/builtin/nodes/ethernet_switch.py index e508649c3..6c11a2238 100644 --- a/gns3server/compute/builtin/nodes/ethernet_switch.py +++ b/gns3server/compute/builtin/nodes/ethernet_switch.py @@ -723,6 +723,13 @@ class EthernetSwitch(KernelDatapathMixin, BaseNode): previous = {port["port_number"]: port for port in (previous_mapping or [])} wired = dict(self._tap_by_port) for port_number, anchor in self._kernel_ports.items(): + if not os.path.exists(f"/sys/class/net/{anchor}"): + # A kernel port whose peer is stopped keeps its registration + # but has no interface yet (the join is deferred by design): + # vlan commands would fail with ENODEV and abort the whole + # reconcile. Its new settings apply when the peer starts — + # the deferred join runs _apply_port_vlan then. + continue wired[port_number] = anchor for port_number, iface in wired.items(): new_settings = self._port_settings(port_number) @@ -761,7 +768,16 @@ class EthernetSwitch(KernelDatapathMixin, BaseNode): if self._ubridge_hypervisor and self._ubridge_hypervisor.is_running(): anchor = self._kernel_ports.get(port_number) if anchor is not None: - await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + # One capture per uBridge process — see + # _reserve_kernel_capture; a failed start rolls the port's + # flag back so it cannot later stop the winner's capture. + try: + self._reserve_kernel_capture(anchor) + await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + except Exception: + self._release_kernel_capture(anchor) + nio.stop_packet_capture() + raise else: ubridge_bridge = self._ubridge_bridge_name(port_number) await self._ubridge_send(f'bridge start_capture {ubridge_bridge} "{output_file}"') @@ -779,8 +795,14 @@ class EthernetSwitch(KernelDatapathMixin, BaseNode): return nio.stop_packet_capture() if self._ubridge_hypervisor and self._ubridge_hypervisor.is_running(): - if port_number in self._kernel_ports: - await self._ubridge_send("capture stop_kernel") + anchor = self._kernel_ports.get(port_number) + if anchor is not None: + # Process-wide and argument-less: only the port owning the + # slot may stop it (a second port's stop would kill this + # capture while its own flag still says capturing). + if self._kernel_capture_owned_by(anchor): + await self._ubridge_send("capture stop_kernel") + self._release_kernel_capture(anchor) else: ubridge_bridge = self._ubridge_bridge_name(port_number) await self._ubridge_send(f"bridge stop_capture {ubridge_bridge}") diff --git a/gns3server/compute/docker/docker_kernel_datapath.py b/gns3server/compute/docker/docker_kernel_datapath.py index 8c8c62b9a..a6300b45d 100644 --- a/gns3server/compute/docker/docker_kernel_datapath.py +++ b/gns3server/compute/docker/docker_kernel_datapath.py @@ -128,11 +128,12 @@ class DockerKernelDatapathMixin(KernelDatapathMixin): await self._ubridge_send(f"docker move_to_ns {guest_ifc} {self._namespace} {ifname}") except UbridgeError as e: raise UbridgeNamespaceError(e) - except UbridgeNamespaceError: - raise except Exception: # Don't leave a half-created pair behind (the next start would - # fail on create_veth EEXIST until the stale sweep runs). + # fail on create_veth EEXIST until the stale sweep runs). The + # namespace failure cleans up the same way — the bare raise + # preserves its type for the caller's UbridgeNamespaceError + # handling (docker_vm.start converts it into a failed start). with contextlib.suppress(UbridgeError): await self._ubridge_send(f'docker delete_veth "{host_ifc}"') raise diff --git a/gns3server/compute/docker/docker_vm.py b/gns3server/compute/docker/docker_vm.py index 4cb34d85e..c5cbbc19c 100644 --- a/gns3server/compute/docker/docker_vm.py +++ b/gns3server/compute/docker/docker_vm.py @@ -1869,6 +1869,14 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode): f"Port {port_number} doesn't exist on adapter {adapter_number} of Docker container '{self.name}'" ) + # The compute-side backstop of the controller's duplicate-port guard: + # a port carries at most one NIO, and overwriting the one a link + # still owns orphans that link's teardown (its host state leaks). + if adapter.get_nio(port_number) is not None: + raise DockerError( + f"Port {port_number} on adapter {adapter_number} of Docker container '{self.name}' already has a link" + ) + if self.status == "started" and self.ubridge: await self._connect_nio(adapter_number, nio, port_number) await self._set_adapter_carrier(adapter_number, not nio.suspend, port_number) @@ -1999,11 +2007,18 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode): if isinstance(nio, NIOBridge): # Kernel datapath: no relay bridge exists — capture via uBridge's # AF_PACKET module bound to the anchor (a veth host end for the - # standard container, a persistent TAP for an IOL runner). - # Single capture per uBridge process: a concurrent second one - # returns EALREADY. + # standard container, a persistent TAP for an IOL runner). One + # capture per uBridge process: the slot is claimed per anchor, + # and a failed start rolls the port's capturing flag back so the + # port cannot later stop the winner's capture. host_ifc = self._kernel_host_ifc(adapter_number, port_number) - await self._ubridge_send(f'capture start_kernel {host_ifc} "{output_file}"') + try: + self._reserve_kernel_capture(host_ifc) + await self._ubridge_send(f'capture start_kernel {host_ifc} "{output_file}"') + except Exception: + self._release_kernel_capture(host_ifc) + nio.stop_packet_capture() + raise return # Relay datapath (even though the adapter interface is a veth, the # traffic flows through the uBridge bridge — key on the NIO type, @@ -2023,8 +2038,13 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode): raise DockerError("Cannot stop the packet capture: uBridge is not running") nio = self._ethernet_adapters[adapter_number].get_nio(port_number) if isinstance(nio, NIOBridge): - # Idempotent on the uBridge side (no active capture is OK). - await self._ubridge_send("capture stop_kernel") + # Idempotent on the uBridge side (no active capture is OK), but + # process-wide and argument-less: only the port owning the slot + # may stop it, or a second port's stop would kill this capture. + host_ifc = self._kernel_host_ifc(adapter_number, port_number) + if self._kernel_capture_owned_by(host_ifc): + await self._ubridge_send("capture stop_kernel") + self._release_kernel_capture(host_ifc) return bridge_name = self._bridge_name(adapter_number, port_number) await self._ubridge_send(f"bridge stop_capture {bridge_name}") diff --git a/gns3server/compute/dynamips/nodes/router.py b/gns3server/compute/dynamips/nodes/router.py index f763d7dd5..de0063d43 100644 --- a/gns3server/compute/dynamips/nodes/router.py +++ b/gns3server/compute/dynamips/nodes/router.py @@ -1486,11 +1486,23 @@ class Router(KernelDatapathMixin, BaseNode): if isinstance(nio, NIOBridge): # Kernel link: capture on the port's TAP anchor (AF_PACKET), # not in the Dynamips NIO (this link has no Dynamips NIO). + if nio.capturing: + raise DynamipsError(f"Packet capture is already activated on port {slot_number}/{port_number}") nio.start_packet_capture(output_file, data_link_type) if self.ubridge: anchor = self._kernel_host_ifc(slot_number, port_number) if anchor is not None: - await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + # One capture per uBridge process — see + # _reserve_kernel_capture; a failed start rolls the + # port's flag back so it cannot later stop the winner's + # capture. + try: + self._reserve_kernel_capture(anchor) + await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + except Exception: + self._release_kernel_capture(anchor) + nio.stop_packet_capture() + raise log.debug( f'Router "{self._name}" [{self._id}]: starting packet capture on port {slot_number}/{port_number}' ) @@ -1526,8 +1538,12 @@ class Router(KernelDatapathMixin, BaseNode): if isinstance(nio, NIOBridge): nio.stop_packet_capture() - if self.ubridge and self._kernel_host_ifc(slot_number, port_number) is not None: + anchor = self._kernel_host_ifc(slot_number, port_number) + if self.ubridge and anchor is not None and self._kernel_capture_owned_by(anchor): + # Process-wide and argument-less: only the port owning the + # slot may stop it. await self._ubridge_send("capture stop_kernel") + self._release_kernel_capture(anchor) return await nio.stop_packet_capture() @@ -1720,8 +1736,21 @@ class Router(KernelDatapathMixin, BaseNode): raise await self.slot_enable_nio(slot_number, port_number) self._tap_nios[(slot_number, port_number)] = tap_nio - await self._kernel_attach(anchor, nio) - await self._set_adapter_carrier(slot_number, not nio.suspend, port_number) + try: + await self._kernel_attach(anchor, nio) + await self._set_adapter_carrier(slot_number, not nio.suspend, port_number) + except Exception: + # Undo the half-attach: while the _tap_nios entry exists the port + # guard above short-circuits every retry, and a link delete reads + # the still-unbound adapter as "nothing to do" — the port would + # stay a silent black hole until the router restarts. The undo + # tolerates the partial state (its marker/bridge steps are + # best-effort) and takes the hypervisor binding and the TAP with + # it, so a retry starts from scratch. It is best-effort here as + # well: the original failure is what must surface. + with contextlib.suppress(Exception): + await self._remove_kernel_nio_binding(slot_number, port_number, nio) + raise async def _remove_kernel_nio_binding(self, slot_number, port_number, nio): """ diff --git a/gns3server/compute/iou/iou_vm.py b/gns3server/compute/iou/iou_vm.py index d3cd01dfc..aad00f0bc 100644 --- a/gns3server/compute/iou/iou_vm.py +++ b/gns3server/compute/iou/iou_vm.py @@ -686,74 +686,87 @@ class IOUVM(KernelDatapathMixin, BaseNode): raise IOUError(f"The iourc path '{iourc_path}' is not a regular file") await self._check_iou_license() - await self._start_ubridge() - await self._prepare_tap_datapath() - self._create_netmap_config() - if self.use_default_iou_values: - # make sure we have the default nvram amount to correctly push the configs - await self.update_default_iou_values() - self._push_configs_to_nvram() - - # check if there is enough RAM to run - self.check_available_ram(self.ram) - - self._nvram_watcher = FileWatcher(self._nvram_file(), self._nvram_changed, delay=2) - - # created an environment variable pointing to the iourc file. - env = os.environ.copy() - if "IOURC" not in os.environ and iourc_path: - env["IOURC"] = iourc_path - - # create a symbolic link to the image to avoid IOU error "failed code signing checks" - # on newer images, see https://github.com/GNS3/gns3-server/issues/1484 + # The try starts before the uBridge spawn: a raise anywhere from + # here to the launch (tap datapath, netmap config, symlink, + # command build) must take the datapath down again — a node that + # reports a failed start must not keep a uBridge process and its + # anchor TAPs alive, since nothing else ever reclaims them. try: - iou_image_path = os.path.basename(self.path) - if len(iou_image_path) > 63: - # IOU file basename length must be <= 63 chars - iou_file_name, iou_file_ext = os.path.splitext(iou_image_path) - iou_image_path = iou_file_name[: 63 - len(iou_file_ext)] + iou_file_ext - symlink = os.path.join(self.working_dir, iou_image_path) - if os.path.islink(symlink): - os.unlink(symlink) - os.symlink(self.path, symlink) - except OSError as e: - raise IOUError(f"Could not create symbolic link: {e}") + await self._start_ubridge() + await self._prepare_tap_datapath() + self._create_netmap_config() + if self.use_default_iou_values: + # make sure we have the default nvram amount to correctly push the configs + await self.update_default_iou_values() + self._push_configs_to_nvram() - command = await self._build_command() - # Only start the responder when the capability probe actually - # enabled IOU's L1 protocol on the command line. - if "-l" in command: - await self._start_l1_keepalive_responder() - try: - if self._loader: - log.debug(f"Starting IOU: {command} with loader {self._loader}") - else: - log.debug(f"Starting IOU: {command}") - self.command_line = " ".join(command) - self._iou_process = await asyncio.create_subprocess_exec( - *self._loader, - *command, - stdout=asyncio.subprocess.PIPE, - stdin=asyncio.subprocess.PIPE, - stderr=subprocess.STDOUT, - cwd=self.working_dir, - env=env, - ) - log.debug(f"IOU instance {self._id} started PID={self._iou_process.pid}") - self._started = True - self.status = "started" - callback = functools.partial(self._termination_callback, "IOU") - gns3server.utils.asyncio.monitor_process(self._iou_process, callback) - except FileNotFoundError as e: - self._stop_l1_keepalive_responder() - raise IOUError( - f"Could not start IOU: {e}: 32-bit binary support is probably not installed, it is recommended to use a 64-bit image instead" - ) - except (OSError, subprocess.SubprocessError) as e: - self._stop_l1_keepalive_responder() - iou_stdout = self.read_iou_stdout() - log.error(f"Could not start IOU {self._path}: {e}\n{iou_stdout}") - raise IOUError(f"Could not start IOU {self._path}: {e}\n{iou_stdout}") + # check if there is enough RAM to run + self.check_available_ram(self.ram) + + self._nvram_watcher = FileWatcher(self._nvram_file(), self._nvram_changed, delay=2) + + # created an environment variable pointing to the iourc file. + env = os.environ.copy() + if "IOURC" not in os.environ and iourc_path: + env["IOURC"] = iourc_path + + # create a symbolic link to the image to avoid IOU error "failed code signing checks" + # on newer images, see https://github.com/GNS3/gns3-server/issues/1484 + try: + iou_image_path = os.path.basename(self.path) + if len(iou_image_path) > 63: + # IOU file basename length must be <= 63 chars + iou_file_name, iou_file_ext = os.path.splitext(iou_image_path) + iou_image_path = iou_file_name[: 63 - len(iou_file_ext)] + iou_file_ext + symlink = os.path.join(self.working_dir, iou_image_path) + if os.path.islink(symlink): + os.unlink(symlink) + os.symlink(self.path, symlink) + except OSError as e: + raise IOUError(f"Could not create symbolic link: {e}") + + command = await self._build_command() + # Only start the responder when the capability probe actually + # enabled IOU's L1 protocol on the command line. + if "-l" in command: + await self._start_l1_keepalive_responder() + try: + if self._loader: + log.debug(f"Starting IOU: {command} with loader {self._loader}") + else: + log.debug(f"Starting IOU: {command}") + self.command_line = " ".join(command) + self._iou_process = await asyncio.create_subprocess_exec( + *self._loader, + *command, + stdout=asyncio.subprocess.PIPE, + stdin=asyncio.subprocess.PIPE, + stderr=subprocess.STDOUT, + cwd=self.working_dir, + env=env, + ) + log.debug(f"IOU instance {self._id} started PID={self._iou_process.pid}") + self._started = True + self.status = "started" + callback = functools.partial(self._termination_callback, "IOU") + gns3server.utils.asyncio.monitor_process(self._iou_process, callback) + except FileNotFoundError as e: + self._stop_l1_keepalive_responder() + raise IOUError( + f"Could not start IOU: {e}: 32-bit binary support is probably not installed, it is recommended to use a 64-bit image instead" + ) + except (OSError, subprocess.SubprocessError) as e: + self._stop_l1_keepalive_responder() + iou_stdout = self.read_iou_stdout() + log.error(f"Could not start IOU {self._path}: {e}\n{iou_stdout}") + raise IOUError(f"Could not start IOU {self._path}: {e}\n{iou_stdout}") + except Exception: + # Re-raise unchanged after the teardown so callers keep the + # original error contract. The L1 responder needs no stop + # here: _start_l1_keepalive_responder cleans up after itself + # and everything after it runs under handlers that stop it. + await self._stop_ubridge() + raise await self.start_console() @@ -1394,10 +1407,14 @@ class IOUVM(KernelDatapathMixin, BaseNode): :param filters: Array of filter dictionnary """ + # The base relay path's guards apply here too: kernel-only filters + # must be a clear NodeError (a direct compute API update would + # otherwise reach iol_bridge and fail there), and a BPF line that no + # longer compiles degrades to a warning instead of a hard error. + self._guard_kernel_only_filters(filters) await self._ubridge_send("iol_bridge reset_packet_filters " + location) for filter in self._build_filter_list(filters): - cmd = f"iol_bridge add_packet_filter {location} {filter}" - await self._ubridge_send(cmd) + await self._ubridge_add_packet_filter(f"iol_bridge add_packet_filter {location} {filter}") async def _ubridge_add_marker_filter( self, location, name, bpf, pcap_path, tag=None, link_id=None, direction=None, data_link_type=None @@ -1826,13 +1843,22 @@ class IOUVM(KernelDatapathMixin, BaseNode): if self.ubridge: if isinstance(nio, NIOBridge): - # Kernel link: capture on the port's TAP anchor (AF_PACKET). + # Kernel link: capture on the port's TAP anchor (AF_PACKET, + # one capture per uBridge process — see _reserve_kernel_capture; + # a failed start rolls the port's flag back so the port cannot + # later stop the winner's capture). anchor = self._kernel_host_ifc(adapter_number, port_number) - if anchor is None: - raise self._kernel_error( - f"Bay {adapter_number}/{port_number} of IOU '{self._name}' has no TAP anchor to capture on" - ) - await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + try: + if anchor is None: + raise self._kernel_error( + f"Bay {adapter_number}/{port_number} of IOU '{self._name}' has no TAP anchor to capture on" + ) + self._reserve_kernel_capture(anchor) + await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + except Exception: + self._release_kernel_capture(anchor) + nio.stop_packet_capture() + raise else: await self._ubridge_send( 'iol_bridge start_capture {name} {bay} {unit} "{output_file}" {data_link_type}'.format( @@ -1859,7 +1885,13 @@ class IOUVM(KernelDatapathMixin, BaseNode): log.debug(f'IOU "{self._name}" [{self._id}]: stopping packet capture on {adapter_number}/{port_number}') if self.ubridge: if isinstance(nio, NIOBridge): - await self._ubridge_send("capture stop_kernel") + # Process-wide and argument-less: only the port owning the + # slot may stop it (a second port's stop would kill this + # capture while its own flag still says capturing). + anchor = self._kernel_host_ifc(adapter_number, port_number) + if self._kernel_capture_owned_by(anchor): + await self._ubridge_send("capture stop_kernel") + self._release_kernel_capture(anchor) else: await self._ubridge_send( f"iol_bridge stop_capture {self._iol_bridge_name()} {adapter_number} {port_number}" diff --git a/gns3server/compute/kernel_datapath.py b/gns3server/compute/kernel_datapath.py index 4a23c2b51..fd5fced43 100644 --- a/gns3server/compute/kernel_datapath.py +++ b/gns3server/compute/kernel_datapath.py @@ -149,6 +149,52 @@ class KernelDatapathMixin: return not (isinstance(nio, NIOBridge) and nio.bridge is None) + # The anchor whose kernel capture currently holds the uBridge process's + # single AF_PACKET capture slot (None = free; see _reserve_kernel_capture). + # Class-level default so every node type gets it without touching its + # __init__; BaseNode._stop_ubridge resets it when uBridge goes away. + _kernel_capture_ifc = None + + def _reserve_kernel_capture(self, anchor): + """ + Claim uBridge's single kernel-capture slot for *anchor*. + + uBridge serves one AF_PACKET capture per process (a static slot: a + second ``capture start_kernel`` answers EALREADY) while the server + models captures per port — a second kernel port of the same node is + refused here with a clear error. Without the claim the loser's port + would stay marked as capturing, and its stop would issue the + process-wide, argument-less ``capture stop_kernel``, killing the + winner's capture while its own flag still says capturing. + """ + + if self._kernel_capture_ifc is not None and self._kernel_capture_ifc != anchor: + raise self._kernel_error( + f"Cannot start the packet capture: this node's uBridge already captures " + f"{self._kernel_capture_ifc} (one kernel capture per node); stop that capture first" + ) + self._kernel_capture_ifc = anchor + + def _release_kernel_capture(self, anchor): + """ + Release the slot if *anchor* holds it: after a stop, or after a + failed start whose port must not stay marked as capturing. + """ + + if self._kernel_capture_ifc == anchor: + self._kernel_capture_ifc = None + + def _kernel_capture_owned_by(self, anchor): + """ + Whether *anchor* may issue the process-wide ``capture stop_kernel``: + only the port holding the slot — or a node whose slot is empty (a + capture predating the tracker), where stopping is the historical + best-effort behaviour. A second port must never reach it: the + command takes no interface and would stop the owner's capture. + """ + + return self._kernel_capture_ifc is None or self._kernel_capture_ifc == anchor + async def _kernel_attach(self, anchor, nio): """ Attach an anchor to the per-link kernel bridge carried by *nio* @@ -181,7 +227,10 @@ class KernelDatapathMixin: await self._ubridge_send(f'brctl addif "{nio.bridge}" "{anchor}"') if nio.capturing: # Restore a capture that was active before a node restart - # (mirrors the relay path's start_capture). + # (mirrors the relay path's start_capture). The slot is claimed + # like a fresh start: a second kernel port's replay is refused + # with a clear error instead of an EALREADY deep in the wiring. + self._reserve_kernel_capture(anchor) await self._ubridge_send(f'capture start_kernel {anchor} "{nio.pcap_output_file}"') # Markers carried by the NIO attach to the anchor (AF_PACKET taps) — # the anchor is the interface, not a relay bridge. @@ -404,40 +453,34 @@ class KernelDatapathMixin: # ------------------------------------------------------------------ @staticmethod - def _netem_command_parts(filters): + def _values_of(filters, key): + """ + One filter's values as a list, tolerating the legacy bare-value + shape ({"packet_loss": 10}). """ - Build the ``tc netem set`` keyword sequence (in the frozen grammar - order: delay/jitter, loss|gemodel, dup, corrupt, reorder, rate, - limit, distribution, seed) plus the set of capability tokens the - sequence needs beyond the original netem surface. Original-surface - keywords (delay, jitter, loss, corrupt, dup) need no probe; the - extension tokens (rate, reorder, gemodel, dist, seed, limit) are - checked against ``tc capabilities`` by the caller. ``correl`` has no - token of its own in the capabilities list, so it gates on "rate" as - the netem-extension build marker. - :param filters: NIO filters dictionary ({"delay": [ms, jitter, dist], ...}) + values = filters.get(key) + if isinstance(values, (list, tuple)): + return list(values) + return [values] if values else [] + + @staticmethod + def _netem_loss_parts(filters): + """ + The loss keyword: gemodel when set, else packet_loss — mutually + exclusive, both mapping to netem's single loss argument. The gemodel + token is returned for the caller's capability check; a non-zero + packet-loss correlation gates on "rate" as the extension build + marker. :returns: (keyword sequence, required extension tokens) """ - def values_of(key): - # tolerate the legacy bare-value shape ({"packet_loss": 10}) - values = filters.get(key) - if isinstance(values, (list, tuple)): - return list(values) - return [values] if values else [] - + values_of = KernelDatapathMixin._values_of parts = [] ext_tokens = set() - delay = values_of("delay") - if delay: - parts.append(f"delay {int(delay[0])}") - if len(delay) > 1 and int(delay[1]): - parts.append(f"jitter {int(delay[1])}") - - gemodel = values_of("gemodel") + gemodel = values_of(filters, "gemodel") if gemodel: segment = f"loss gemodel {int(gemodel[0])}" if len(gemodel) > 1: @@ -446,26 +489,71 @@ class KernelDatapathMixin: segment += f" {int(gemodel[2])}" parts.append(segment) ext_tokens.add("gemodel") - else: - loss = values_of("packet_loss") - if loss and int(loss[0]): - parts.append(f"loss {int(loss[0])}") - if len(loss) > 1 and int(loss[1]): - parts.append(f"correl {int(loss[1])}") - ext_tokens.add("rate") # correl: extension build marker + return parts, ext_tokens - duplicate = values_of("duplicate") + loss = values_of(filters, "packet_loss") + if loss and int(loss[0]): + parts.append(f"loss {int(loss[0])}") + if len(loss) > 1 and int(loss[1]): + parts.append(f"correl {int(loss[1])}") + ext_tokens.add("rate") # correl: extension build marker + return parts, ext_tokens + + @staticmethod + def _netem_surface_parts(filters): + """ + The original netem surface in its frozen grammar order — delay/ + jitter, loss (or gemodel), dup (+correl), corrupt — plus the + capability tokens it needs. Original-surface keywords need no + probe; ``correl`` has no token of its own in the capabilities + list, so it gates on "rate" as the netem-extension build marker. + + :returns: (keyword sequence, required extension tokens) + """ + + values_of = KernelDatapathMixin._values_of + parts = [] + ext_tokens = set() + + delay = values_of(filters, "delay") + if delay: + parts.append(f"delay {int(delay[0])}") + if len(delay) > 1 and int(delay[1]): + parts.append(f"jitter {int(delay[1])}") + + loss_parts, loss_tokens = KernelDatapathMixin._netem_loss_parts(filters) + parts.extend(loss_parts) + ext_tokens |= loss_tokens + + duplicate = values_of(filters, "duplicate") if duplicate and int(duplicate[0]): parts.append(f"dup {int(duplicate[0])}") if len(duplicate) > 1 and int(duplicate[1]): parts.append(f"correl {int(duplicate[1])}") ext_tokens.add("rate") # correl: extension build marker - corrupt = values_of("corrupt") + corrupt = values_of(filters, "corrupt") if corrupt and int(corrupt[0]): parts.append(f"corrupt {int(corrupt[0])}") - reorder = values_of("reorder") + return parts, ext_tokens + + @staticmethod + def _netem_extension_parts(filters): + """ + The netem extensions in their frozen grammar order — reorder, rate, + limit, distribution, seed — plus each keyword's capability token. + The tokens (rate, reorder, dist, seed, limit) are checked against + ``tc capabilities`` by the caller. + + :returns: (keyword sequence, required extension tokens) + """ + + values_of = KernelDatapathMixin._values_of + parts = [] + ext_tokens = set() + + reorder = values_of(filters, "reorder") if reorder: segment = f"reorder {int(reorder[0])}" if len(reorder) > 1 and int(reorder[1]): @@ -475,23 +563,24 @@ class KernelDatapathMixin: parts.append(segment) ext_tokens.add("reorder") - rate = values_of("rate") + rate = values_of(filters, "rate") if rate and str(rate[0]).strip(): parts.append(f"rate {str(rate[0]).strip()}") ext_tokens.add("rate") - limit = values_of("limit") + limit = values_of(filters, "limit") if limit and int(limit[0]): parts.append(f"limit {int(limit[0])}") ext_tokens.add("limit") + delay = values_of(filters, "delay") if delay and len(delay) > 2 and str(delay[2]).strip().lower() not in ("", "uniform"): # "uniform" is the kernel default — emitting nothing keeps the # command compatible with the original netem surface. parts.append(f"distribution {str(delay[2]).strip().lower()}") ext_tokens.add("dist") - seed = values_of("seed") + seed = values_of(filters, "seed") if seed and int(seed[0]): # 0 is the "disabled" convention everywhere else — treat it so # here too (the controller's inactive-filter pass drops it). @@ -500,6 +589,25 @@ class KernelDatapathMixin: return parts, ext_tokens + @staticmethod + def _netem_command_parts(filters): + """ + Build the ``tc netem set`` keyword sequence (in the frozen grammar + order: delay/jitter, loss|gemodel, dup, corrupt, reorder, rate, + limit, distribution, seed) plus the set of capability tokens the + sequence needs beyond the original netem surface. + + :param filters: NIO filters dictionary ({"delay": [ms, jitter, dist], ...}) + + :returns: (keyword sequence, required extension tokens) + """ + + parts, ext_tokens = KernelDatapathMixin._netem_surface_parts(filters) + extension_parts, extension_tokens = KernelDatapathMixin._netem_extension_parts(filters) + parts.extend(extension_parts) + ext_tokens |= extension_tokens + return parts, ext_tokens + async def _ubridge_apply_netem(self, host_ifc, filters): """ Translate the NIO's impairment filters into one tc netem qdisc on the @@ -523,7 +631,12 @@ class KernelDatapathMixin: :param filters: NIO filters dictionary ({"delay": [ms, jitter], ...}) """ - parts, ext_tokens = self._netem_command_parts(filters) + try: + parts, ext_tokens = self._netem_command_parts(filters) + except (TypeError, ValueError) as e: + # int() on a stray value from an unvalidated direct-API update + # (e.g. {"delay": ["abc"]}) would otherwise surface as a 500. + raise self._kernel_error(f"Malformed packet filter values on a kernel-datapath link: {e}") if parts and ext_tokens: # Extension keywords need the netem-extension uBridge. Plain # delay/loss/corrupt/dup never probes — an old uBridge serves @@ -587,7 +700,13 @@ class KernelDatapathMixin: """ bpf = filters.get("bpf") - lines = [line.strip() for line in (bpf[0].split("\n") if bpf else []) if line.strip()] + # Accept the bare-value shape ({"bpf": "icmp"}): a str is the + # expression itself — indexing it would install its first character. + if isinstance(bpf, str): + bpf = [bpf] + elif not isinstance(bpf, (list, tuple)): + bpf = [] + lines = [line.strip() for line in (str(bpf[0]).split("\n") if bpf else []) if line.strip()] if not lines and self._ubridge_tc_caps is None: # Nothing to add and nothing was ever installed (no tc module # probed yet) — skip touching an old uBridge entirely. @@ -653,16 +772,32 @@ class KernelDatapathMixin: :param filters: NIO filters dictionary """ - def values_of(key): - values = filters.get(key) - if isinstance(values, (list, tuple)): - return list(values) - return [values] if values else [] + def numeric_values(key, minimum, maximum): + """ + The filter's values as ints, refusing a malformed shape here: the + direct compute API is unvalidated (the controller's validation + does not run for it), and indexing a short list or int()-ing a + stray string further down would surface as a bare IndexError or + ValueError 500 after the bridge/netem state was already applied. + """ - frequency = values_of("frequency_drop") - quota = values_of("quota") - window = values_of("window_drop") - requested = {filter_type: values_of(filter_type) for filter_type in FILTER_EBPF_MODES} + values = self._values_of(filters, key) + if not values: + return [] + try: + numbers = [int(value) for value in values] + except (TypeError, ValueError): + raise self._kernel_error(f"Packet filter '{key}' needs integer values, got {values!r}") + if not minimum <= len(numbers) <= maximum: + raise self._kernel_error( + f"Packet filter '{key}' needs between {minimum} and {maximum} integer values, got {len(numbers)}" + ) + return numbers + + frequency = numeric_values("frequency_drop", 1, 1) + quota = numeric_values("quota", 2, 2) + window = numeric_values("window_drop", 3, 5) + requested = {filter_type: self._values_of(filters, filter_type) for filter_type in FILTER_EBPF_MODES} if not any(requested.values()) and self._ubridge_tc_caps is None: # Nothing to set and nothing was ever installed (no probe yet) — # skip touching an old uBridge entirely. @@ -672,41 +807,67 @@ class KernelDatapathMixin: # (ebpf_modes); builds predating the field keep their shipped modes. modes = usable_ebpf_modes(caps) if not any(requested.values()): - # Only modes this build declares can ever have been installed — - # sending "off" for an undeclared mode would hit an unknown - # command; "off" is idempotent in uBridge. - for filter_type, mode in FILTER_EBPF_MODES.items(): - if mode in modes: - await self._ubridge_send(f'tc {mode}_drop "{host_ifc}" off') + await self._ebpf_turn_off_modes(host_ifc, modes) return unsupported = sorted(f for f, values in requested.items() if values and FILTER_EBPF_MODES[f] not in modes) if unsupported: - if not modes: - raise self._kernel_error( - "Packet filter(s) {} on a kernel-datapath link need a uBridge with eBPF support " - "(tc capabilities reports no ebpf; setcap cap_bpf,cap_net_admin,cap_net_raw=ep on " - "the uBridge binary); upgrade uBridge on this compute or keep the link on the " - "relay datapath".format(", ".join(unsupported)) - ) + self._reject_unsupported_ebpf_modes(unsupported, modes) + await self._ebpf_apply_modes(host_ifc, frequency, quota, window, modes) + + async def _ebpf_turn_off_modes(self, host_ifc, modes): + """ + Turn off every mode this build declares. Only declared modes can + ever have been installed — sending "off" for an undeclared mode + would hit an unknown command; "off" is idempotent in uBridge. + """ + + for mode in FILTER_EBPF_MODES.values(): + if mode in modes: + await self._ubridge_send(f'tc {mode}_drop "{host_ifc}" off') + + def _reject_unsupported_ebpf_modes(self, unsupported, modes): + """ + Refuse filters whose mode this uBridge cannot run: a clear error + naming the missing capability (no eBPF at all vs. an older build) + beats a late uBridge failure. + """ + + if not modes: raise self._kernel_error( - "Packet filter(s) {} on a kernel-datapath link need a newer uBridge (tc capabilities " - "reports ebpf_modes={}); upgrade uBridge on this compute or keep the link on the " - "relay datapath".format(", ".join(unsupported), ",".join(modes)) + "Packet filter(s) {} on a kernel-datapath link need a uBridge with eBPF support " + "(tc capabilities reports no ebpf; setcap cap_bpf,cap_net_admin,cap_net_raw=ep on " + "the uBridge binary); upgrade uBridge on this compute or keep the link on the " + "relay datapath".format(", ".join(unsupported)) ) + raise self._kernel_error( + "Packet filter(s) {} on a kernel-datapath link need a newer uBridge (tc capabilities " + "reports ebpf_modes={}); upgrade uBridge on this compute or keep the link on the " + "relay datapath".format(", ".join(unsupported), ",".join(modes)) + ) + + async def _ebpf_apply_modes(self, host_ifc, frequency, quota, window, modes): + """ + Set the requested modes (and clear the absent ones) in the + classifier's fixed order nth → quota → window. A mode left absent + is turned off explicitly (idempotent) so a removed filter stops + dropping; for window_drop this means the schedule restarts on every + apply (the first window opens Start-ms from the moment of the call). + """ + if frequency: # relay semantics: -1 = drop everything, N = every Nth packet - nth = 1 if int(frequency[0]) == -1 else int(frequency[0]) + nth = 1 if frequency[0] == -1 else frequency[0] await self._ubridge_send(f'tc nth_drop "{host_ifc}" {nth}') elif "nth" in modes: await self._ubridge_send(f'tc nth_drop "{host_ifc}" off') if quota: - await self._ubridge_send(f'tc quota_drop "{host_ifc}" {int(quota[0])} {int(quota[1])}') + await self._ubridge_send(f'tc quota_drop "{host_ifc}" {quota[0]} {quota[1]}') elif "quota" in modes: await self._ubridge_send(f'tc quota_drop "{host_ifc}" off') if window: # [start, outage, chance, period?, jitter?] — start is relative # to this apply (the schedule restarts on every reconcile). - params = " ".join(str(int(v)) for v in window) + params = " ".join(str(v) for v in window) await self._ubridge_send(f'tc window_drop "{host_ifc}" {params}') elif "window" in modes: await self._ubridge_send(f'tc window_drop "{host_ifc}" off') diff --git a/gns3server/compute/qemu/qemu_vm.py b/gns3server/compute/qemu/qemu_vm.py index db842f781..37485fc53 100644 --- a/gns3server/compute/qemu/qemu_vm.py +++ b/gns3server/compute/qemu/qemu_vm.py @@ -1132,13 +1132,20 @@ class QemuVM(KernelDatapathMixin, BaseNode): # created — and before QEMU opens them. Unlike a container, whose # veths are created after it starts, the TAP has to pre-exist the # process that reads it. + # + # The try starts here rather than at the launch: every step up to + # the launch can fail (a broken qemu_path in _build_command, a + # tap create refused), and each of those failures must take the + # datapath down again — a node that reports stopped must not keep + # a uBridge process and root-namespace TAPs alive, since nothing + # else ever reclaims them. self._ubridge_tc_caps = None - await self._start_ubridge() - await self._prepare_tap_datapath() - - command = await self._build_command() - command_string = " ".join(shlex.quote(s) for s in command) try: + await self._start_ubridge() + await self._prepare_tap_datapath() + + command = await self._build_command() + command_string = " ".join(shlex.quote(s) for s in command) log.debug(f"Starting QEMU with: {command_string}") self._stdout_file = os.path.join(self.working_dir, "qemu.log") log.debug(f"logging to {self._stdout_file}") @@ -1159,6 +1166,12 @@ class QemuVM(KernelDatapathMixin, BaseNode): # the launch: don't leave either behind on a failed start. await self._stop_ubridge() raise QemuError(f"Could not start QEMU {self.qemu_path}: {e}\n{stdout}") + except Exception: + # The same teardown for a failure raised as anything else + # (QemuError from _build_command, UbridgeError from the tap + # setup) — re-raised unchanged so callers keep their contract. + await self._stop_ubridge() + raise await self._set_process_priority() if self._cpu_throttling: @@ -1238,60 +1251,63 @@ class QemuVM(KernelDatapathMixin, BaseNode): async with self._execute_lock: # stop the QEMU process self._hw_virtualization = False - if self.is_running(): - log.debug(f'Stopping QEMU VM "{self._name}" PID={self._process.pid}') - try: - if self.on_close == "save_vm_state": - await self._control_vm("stop") - await self._control_vm("savevm GNS3_SAVED_STATE") - wait_for_savevm = 120 - while wait_for_savevm: - await asyncio.sleep(1) - status = await self._saved_state_option() - wait_for_savevm -= 1 - if status != []: - break + try: + if self.is_running(): + log.debug(f'Stopping QEMU VM "{self._name}" PID={self._process.pid}') + try: + if self.on_close == "save_vm_state": + await self._control_vm("stop") + await self._control_vm("savevm GNS3_SAVED_STATE") + wait_for_savevm = 120 + while wait_for_savevm: + await asyncio.sleep(1) + status = await self._saved_state_option() + wait_for_savevm -= 1 + if status != []: + break - if self.on_close == "shutdown_signal": - await self._control_vm("system_powerdown") - await gns3server.utils.asyncio.wait_for_process_termination(self._process, timeout=120) - else: - self._process.terminate() - await gns3server.utils.asyncio.wait_for_process_termination(self._process, timeout=3) - except ProcessLookupError: - pass - except asyncio.TimeoutError: - if self._process: - try: - self._process.kill() - except ProcessLookupError: - pass - if self._process.returncode is None: - log.warning(f'QEMU VM "{self._name}" PID={self._process.pid} is still running') - self._process = None - self._stop_cpulimit() - self._stop_swtpm() - if self.on_close != "save_vm_state": - await self._clear_save_vm_stated() - await self._export_config() - await super().stop() - - # Release the datapath only once QEMU is gone: the process holds - # the adapter TAPs open (it is the guest side), and uBridge's tap - # delete refuses a held device ("Device or resource busy") — - # deleting them while QEMU still ran leaked the persistent TAPs, - # because that best-effort delete is suppressed. Order: process - # side first, then the devices (the same lesson as IOU's reverse - # stop order). - # - # It stays under the lock: the process monitor calls stop() on its - # own when QEMU dies, so an API stop and a process-death stop run - # concurrently in the normal case — and the TAP sweep walks the - # adapter map across awaits, which the second stop was free to - # clear underneath it ("dictionary changed size during - # iteration"). Serialized, it finds uBridge already gone and - # sweeps nothing. - await self._stop_ubridge() + if self.on_close == "shutdown_signal": + await self._control_vm("system_powerdown") + await gns3server.utils.asyncio.wait_for_process_termination(self._process, timeout=120) + else: + self._process.terminate() + await gns3server.utils.asyncio.wait_for_process_termination(self._process, timeout=3) + except ProcessLookupError: + pass + except asyncio.TimeoutError: + if self._process: + try: + self._process.kill() + except ProcessLookupError: + pass + if self._process.returncode is None: + log.warning(f'QEMU VM "{self._name}" PID={self._process.pid} is still running') + self._process = None + self._stop_cpulimit() + self._stop_swtpm() + if self.on_close != "save_vm_state": + await self._clear_save_vm_stated() + await self._export_config() + await super().stop() + finally: + # Release the datapath once QEMU is gone: the process holds + # the adapter TAPs open (it is the guest side), and uBridge's + # tap delete refuses a held device ("Device or resource + # busy") — deleting them while QEMU still ran leaked the + # persistent TAPs, because that best-effort delete is + # suppressed. Order: process side first, then the devices + # (the same lesson as IOU's reverse stop order). The finally + # keeps a mid-stop raise (a failed savevm, an export error) + # from skipping the teardown entirely. + # + # It stays under the lock: the process monitor calls stop() on + # its own when QEMU dies, so an API stop and a process-death + # stop run concurrently in the normal case — and the TAP + # sweep walks the adapter map across awaits, which the second + # stop was free to clear underneath it ("dictionary changed + # size during iteration"). Serialized, it finds uBridge + # already gone and sweeps nothing. + await self._stop_ubridge() async def _open_qemu_monitor_connection_vm(self, timeout=10): """ @@ -1665,6 +1681,12 @@ class QemuVM(KernelDatapathMixin, BaseNode): except IndexError: raise QemuError(f'Adapter {adapter_number} does not exist on QEMU VM "{self._name}"') + # The compute-side backstop of the controller's duplicate-port guard: + # a port carries at most one NIO, and overwriting the one a link + # still owns orphans that link's teardown (its host state leaks). + if adapter.get_nio(0) is not None: + raise QemuError(f'Adapter {adapter_number} of QEMU VM "{self._name}" already has a link') + if self.is_running(): try: await self._connect_nio(adapter_number, nio) @@ -1777,8 +1799,17 @@ class QemuVM(KernelDatapathMixin, BaseNode): if self.ubridge: anchor = self._kernel_host_ifc(adapter_number) if isinstance(nio, NIOBridge) and anchor is not None: - # Kernel link: capture on the adapter's TAP anchor (AF_PACKET). - await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + # Kernel link: capture on the adapter's TAP anchor (AF_PACKET, + # one capture per uBridge process — see _reserve_kernel_capture; + # a failed start rolls the port's flag back so the port cannot + # later stop the winner's capture). + try: + self._reserve_kernel_capture(anchor) + await self._ubridge_send(f'capture start_kernel {anchor} "{output_file}"') + except Exception: + self._release_kernel_capture(anchor) + nio.stop_packet_capture() + raise else: await self._ubridge_send( 'bridge start_capture {name} "{output_file}"'.format( @@ -1801,8 +1832,14 @@ class QemuVM(KernelDatapathMixin, BaseNode): nio.stop_packet_capture() if self.ubridge: - if isinstance(nio, NIOBridge) and self._kernel_host_ifc(adapter_number) is not None: - await self._ubridge_send("capture stop_kernel") + anchor = self._kernel_host_ifc(adapter_number) + if isinstance(nio, NIOBridge) and anchor is not None: + # Process-wide and argument-less: only the port owning the + # slot may stop it (a second port's stop would kill this + # capture while its own flag still says capturing). + if self._kernel_capture_owned_by(anchor): + await self._ubridge_send("capture stop_kernel") + self._release_kernel_capture(anchor) else: await self._ubridge_send("bridge stop_capture {name}".format(name=f"QEMU-{self._id}-{adapter_number}")) diff --git a/gns3server/compute/ubridge/tc_probe.py b/gns3server/compute/ubridge/tc_probe.py index fdff74d6d..4322a53ee 100644 --- a/gns3server/compute/ubridge/tc_probe.py +++ b/gns3server/compute/ubridge/tc_probe.py @@ -49,14 +49,16 @@ from .ubridge_error import UbridgeError log = logging.getLogger(__name__) -# binary identity -> parsed report (None = probed and unusable) -_cache: dict[tuple[str, int, int], Optional[dict[str, str]]] = {} +# binary identity (path, mtime, size, ctime — ctime catches chmod/setcap, +# which change neither mtime nor size) -> parsed report (None = probed and +# unusable) +_cache: dict[tuple[str, int, int, int], Optional[dict[str, str]]] = {} # binary identity -> whether the tap module works (None = unknown) -_tap_cache: dict[tuple[str, int, int], Optional[bool]] = {} +_tap_cache: dict[tuple[str, int, int, int], Optional[bool]] = {} # binary identity -> whether iol_bridge can bind a port to a TAP (None = unknown) -_iol_tap_cache: dict[tuple[str, int, int], Optional[bool]] = {} +_iol_tap_cache: dict[tuple[str, int, int, int], Optional[bool]] = {} # binary identity -> whether the bridge module can release a named TAP NIO (None = unknown) -_bridge_tap_cache: dict[tuple[str, int, int], Optional[bool]] = {} +_bridge_tap_cache: dict[tuple[str, int, int, int], Optional[bool]] = {} _lock = asyncio.Lock() @@ -74,7 +76,7 @@ async def probe_tc_capabilities(timeout: float = 15.0): return None try: stat = os.stat(path) - key = (path, stat.st_mtime_ns, stat.st_size) + key = (path, stat.st_mtime_ns, stat.st_size, stat.st_ctime_ns) except OSError: return None @@ -137,7 +139,7 @@ async def probe_tap_support(timeout: float = 15.0): return None try: stat = os.stat(path) - key = (path, stat.st_mtime_ns, stat.st_size) + key = (path, stat.st_mtime_ns, stat.st_size, stat.st_ctime_ns) except OSError: return None @@ -215,7 +217,7 @@ async def probe_iol_tap_support(timeout: float = 15.0): return None try: stat = os.stat(path) - key = (path, stat.st_mtime_ns, stat.st_size) + key = (path, stat.st_mtime_ns, stat.st_size, stat.st_ctime_ns) except OSError: return None @@ -297,7 +299,7 @@ async def probe_bridge_tap_support(timeout: float = 15.0): return None try: stat = os.stat(path) - key = (path, stat.st_mtime_ns, stat.st_size) + key = (path, stat.st_mtime_ns, stat.st_size, stat.st_ctime_ns) except OSError: return None @@ -346,7 +348,10 @@ async def _probe_bridge_tap(path, config, timeout): try: return await asyncio.wait_for(ask(), timeout=timeout) - except (OSError, asyncio.TimeoutError, ValueError) as e: + except (UbridgeError, OSError, asyncio.TimeoutError, ValueError) as e: + # UbridgeError also covers hypervisor.start()/connect() failing + # (unlike the siblings, ask() re-raises those): an unusable + # binary means unknown, never a failed /capabilities response. log.debug("uBridge bridge-tap probe failed: %s", e) return None finally: diff --git a/gns3server/controller/link.py b/gns3server/controller/link.py index 909792ac8..6d2fa355e 100644 --- a/gns3server/controller/link.py +++ b/gns3server/controller/link.py @@ -803,14 +803,18 @@ class Link: if not reported: return True for caps in reported: + # Field-wise .get: the controller stores the compute's raw + # capabilities verbatim, so a divergent or hand-crafted payload + # with a partial ubridge_tc dict must read as "not supported" + # rather than raising a KeyError into the API response. if filter_type in FILTER_EBPF_MODES: - if FILTER_EBPF_MODES[filter_type] not in caps["ebpf_modes"]: + if FILTER_EBPF_MODES[filter_type] not in (caps.get("ebpf_modes") or []): return False elif filter_type == "bpf": - if not caps["cbpf"]: + if not caps.get("cbpf"): return False elif filter_type in FILTER_NETEM_KEYWORDS: - if FILTER_NETEM_KEYWORDS[filter_type] not in caps["netem"]: + if FILTER_NETEM_KEYWORDS[filter_type] not in (caps.get("netem") or []): return False # the remaining types (delay, packet_loss, corrupt) run on any # build that reports tc capabilities at all diff --git a/gns3server/controller/udp_link.py b/gns3server/controller/udp_link.py index 8e607f1b5..925cbd569 100644 --- a/gns3server/controller/udp_link.py +++ b/gns3server/controller/udp_link.py @@ -70,7 +70,17 @@ def _is_unix_socket_docker(node): """ environment = (node.properties or {}).get("environment") or "" - return "GNS3_UNIX_SOCKET_NIO" in environment and not is_iol_runner_environment(environment) + if is_iol_runner_environment(environment): + return False + for line in environment.splitlines(): + line = line.strip().rstrip(",") + if line.startswith("GNS3_UNIX_SOCKET_NIO="): + # Mirror the compute's parsing exactly (VendorDockerVM): the + # marker must start the line, and only a truthy value enables + # the unix-socket wiring — with 0/false/no the container wires + # adapters the standard way and can carry a kernel link. + return line.split("=", 1)[1].strip().lower() in ("1", "true", "yes") + return False class UDPLink(Link): diff --git a/gns3server/schemas/compute/nios.py b/gns3server/schemas/compute/nios.py index d3be033e1..a41b7eda1 100644 --- a/gns3server/schemas/compute/nios.py +++ b/gns3server/schemas/compute/nios.py @@ -91,8 +91,11 @@ class BridgeNIO(BaseModel): """ type: BridgeNIOType + # Required-but-nullable: the NIO factories read nio_settings["bridge"], + # and the create/update routes drop unset fields — an omitted key would + # pass validation and then surface as a KeyError (500) during binding. bridge: Optional[str] = Field( - None, + ..., description="Kernel bridge name e.g. gns3a1b2c3d4e5; None when an Ethernet switch " "owns the anchor's bridge membership (absorbed link)", ) diff --git a/gns3server/utils/kernel_anchor.py b/gns3server/utils/kernel_anchor.py index 9cf72b77e..c96cddffa 100644 --- a/gns3server/utils/kernel_anchor.py +++ b/gns3server/utils/kernel_anchor.py @@ -59,12 +59,18 @@ def kernel_anchor_type(node_type, environment=None): its own key unchanged. """ - # The marker check mirrors utils.application_id.is_iol_runner_environment - # inline: this module sits below every node module (compute and - # controller alike import it during their own import), so it must not - # depend on anything that pulls the controller package in. - if node_type == "docker" and "GNS3_IOL_RUNNER=" in (environment or ""): - return "iol_docker" + # The marker check mirrors the compute's class selection + # (compute/docker/__init__.py, _select_node_class) inline: only a + # stripped, comma-trimmed line *beginning* with the marker selects + # IOLDockerVM, so e.g. NOTE=GNS3_IOL_RUNNER=1 must not be named as an + # IOL TAP anchor here. The check stays dependency-free on purpose: this + # module sits below every node module (compute and controller alike + # import it during their own import), so it must not import them back. + if node_type == "docker": + for line in (environment or "").splitlines(): + line = line.strip().rstrip(",") + if line.startswith("GNS3_IOL_RUNNER="): + return "iol_docker" return node_type diff --git a/gns3server/utils/packet_filter_validation.py b/gns3server/utils/packet_filter_validation.py index cee97e59c..036921f2d 100644 --- a/gns3server/utils/packet_filter_validation.py +++ b/gns3server/utils/packet_filter_validation.py @@ -399,7 +399,12 @@ def split_kernel_only_features(filters: Dict[str, List[Any]]) -> Tuple[Dict[str, correl = 0 if correl: dropped.add("packet_loss correlation") - values = values[:1] + # The correlation slot is stripped even when it is zero: the + # relay filter takes exactly one argument (ubridge's + # packet_loss_setup rejects argc != 1), so a kept "0" would make + # the whole filter fail to load — only a non-zero value is worth + # reporting as a dropped feature. + values = values[:1] clean[filter_type] = values return clean, dropped diff --git a/pyproject.toml b/pyproject.toml index fc2d9b861..425402005 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -184,7 +184,6 @@ extend-immutable-calls = [ "gns3server/compute/dynamips/nodes/router.py" = ["F522", "RUF006"] "gns3server/compute/iou/iou_vm.py" = ["C901", "S108", "S324"] "gns3server/compute/iou/utils/iou_export.py" = ["C901", "RUF003"] -"gns3server/compute/kernel_datapath.py" = ["C901"] "gns3server/compute/project.py" = ["DTZ006", "S324"] "gns3server/compute/qemu/qemu_vm.py" = ["ASYNC220", "C901", "RUF003", "S603"] "gns3server/compute/ubridge/ubridge_hypervisor.py" = ["C901"] @@ -254,7 +253,6 @@ extend-immutable-calls = [ "tests/controller/test_project.py" = ["RUF015"] "tests/controller/test_snapshot.py" = ["RUF015"] "tests/controller/test_udp_link.py" = ["B006"] -"tests/e2e/test_docker_link_local_frames.py" = ["S603"] "tests/test_server.py" = ["E722"] "tests/test_topologies.py" = ["B011"] "tests/test_utils.py" = ["F403", "F405"]