support clients that close and reconnect their vif (fixes #230) - #232
Open
arkenoi wants to merge 6 commits into
Open
support clients that close and reconnect their vif (fixes #230)#232arkenoi wants to merge 6 commits into
arkenoi wants to merge 6 commits into
Conversation
Two defects on the client-handover path. An HVM reaches both routinely: its two vifs carry one IP, the stubdomain's is admitted first, and the guest's own vif is still parked in add_client when the PV driver arms the emulated-NIC unplug and forces a reboot. 1. remove_client removed by IP without checking identity, so cleanup for a vif that was never admitted evicted the live client holding that IP. The assert guarding it then fired on the NEXT removal - and Cleanup.cleanup is a bare List.iter, so that exception aborted every remaining cleanup handler. Dao.VifMap.iter walks in key order, so the waiting guest vif (lower domid) is cleaned before the stubdomain's: exactly the bad order. 2. A parked admission outlived the vif it was admitting. When the IP was finally freed it registered an interface whose device no longer existed, and the next client for that IP then waited behind a dead one for ever. Admission is now cancelled from cleanup; handlers run last-registered first, so the cancel precedes the removal. Neither is the mirage#230 handshake bug, but both sit on the path the fix for it makes routine.
A client's connection and its vif have different lifetimes. The vif lives from domain build to detach; a connection over it can close and reopen any number of times - a Windows guest does exactly that. The only teardown trigger used to be the vif's directory disappearing, so a closed connection freed nothing (the IP stayed held) and was never remade. Per-connection state now lives in its own Cleanup.t, released the moment the connection closes, and the vif is served again immediately. The outer cleanup_tasks keeps the vif's real lifetime and stops the loop when the directory is gone for good.
An exception escaping Lwt.async reaches Lwt's async_exception_hook and takes the whole firewall down - it is not confined to the client that raised it. Three paths could do that, and reconnection made all three reachable in ordinary use: - the re-serve loop: when a guest shuts down its frontend directory disappears and the next handshake raises Xs_protocol.Error, so an unguarded re-serve turned every ordinary guest reboot into a firewall crash; - conf_vif's listener: it re-raises anything that is not Lwt.Canceled, including the Netif error or_raise throws when the ring goes away - which now happens whenever a guest closes its device, not only at domain death; - wait_clients: its fallback arm re-raised into Lwt.async by construction. All three now log and stop serving that client. wait_clients still handles the vif's real disappearance.
An HVM has two vifs that are indistinguishable in the log: both carry the same
IP and differ only by domid, so a routine handover reads like two clients
fighting over one address:
add client vif {domid=2682} with IP 10.137.0.72
Client 2682:0 (IP: 10.137.0.72) ready
add client vif {domid=2681} with IP 10.137.0.72
That ambiguity cost real debugging time - it is what made the same-IP collision
look like a plausible cause of the Windows hang, which it was not.
Report the backend's type key alongside. Verbatim, not translated: the naming
runs the opposite way to the obvious reading - the GUEST's own vif is the one
marked vif_ioemu, because it is the one with an emulated counterpart, while the
device model's is a plain vif - and a PV-only guest's vif is also a plain vif,
so any friendlier label would be an inference rather than a fact.
Logging only. Nothing routes on the type, and an unreadable key reports '?'
rather than failing the client.
The re-serve handler treated every exception it did not recognise as terminal. That was wrong twice over. It abandoned vifs that still exist. A connection attempt can fail transiently - a guest resetting mid-handshake leaves a revoked grant (Import.map_exn raises), a stale event-channel port (bind_interdomain raises), or half-torn-down ring keys (Enoent from the tx-ring-ref/rx-ring-ref/event-channel reads). None of those are Xs_protocol.Error, so all landed in the terminal arm. wait_clients only re-adds a vif whose directory is ABSENT from its map, so a vif that still exists is never re-offered: that client had no network until it was detached and reattached, or the firewall restarted, announced by a single warn line. And it misclassified the benign case. wait_frontend_ready reports a vanished FRONTEND directory as Xs_protocol.Error, but a BACKEND directory removed first surfaces as a raw Xs_protocol.Enoent - a different constructor - so an ordinary guest death was logged as 'giving up' rather than recognised as gone. Now: both spellings of gone are terminal, anything else is retried up to ten times, and the loop is damped. The delay is not politeness - a crash-looping driver, or a client cycling its guest-writable state key, would otherwise drive back-to-back handshake+teardown cycles (~15 xenstore round trips, two ring map/unmaps and an event-channel bind each) at xenstored speed, against dom0, from a 32 MB unikernel.
… hashsum config.ml pinned ~min:"2.1.8", whose make_backend signature is domid:int -> device_id:int -> t Lwt.t while dispatcher.ml now calls it as ?on_closed:(unit -> unit Lwt.t) -> domid:int -> device_id:int -> unit -> t Lwt.t and S.CONFIGURATION gains wait_frontend_ready. Both are compile-breaking, so the tree only built here because of an untracked duniverse copy: resolved from opam against 2.1.8 it is a FTBFS. The library changes must therefore be released before this lands - the two PRs are ordered, library first. qubes-firewall.sha256 still held the hash of the UNPATCHED build, which the repo's own CI compares verbatim (.github/workflows/docker.yml) and exits 42 on mismatch, so the PR would have failed its first run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With the mirage-net-xen changes, a client's connection and its vif no longer have the same
lifetime: a vif lives from domain build to detach, while a connection over it can close and reopen
any number of times. A Windows guest does exactly that — it closes the device at PnP start and on
every driver restart.
1.
client_eth: remove only our own IP entry, and make admission cancellable.remove_clientremoved by IP without checking identity, so cleanup for a client that was neveradmitted evicted the live one holding that IP; the assert then fired on the next removal, and
Cleanup.cleanupis a bareList.iter, so that exception truncated the remaining handlers. AnHVM reaches this routinely: its two vifs carry the same IP, and
Dao.VifMap.itercleans the lowerdomid first.
2.
dispatcher: serve a vif across many connections.Per-connection state now lives in its own
Cleanup.t, released the moment the connection closesso the IP frees immediately, and the vif is served again. The outer cleanup keeps the vif's real
lifetime.
3.
dispatcher: never let a client's failure escape intoLwt.async.An escaping exception reaches
async_exception_hookand takes down the whole firewall, not oneclient — and reconnection makes three such paths routine: the re-serve loop (a guest shutdown
raises
Xs_protocol.Error),conf_vif's listener (re-raises anything notLwt.Canceled,including the
Netiferror thrown when the ring goes away), andwait_clients' fallback arm(re-raised by construction). The latter two are pre-existing; reconnect merely made them reachable
on every guest reboot.
4.
dispatcher: log which vif a client is.An HVM's two vifs are otherwise indistinguishable in the log — same IP, differing only by domid —
so a routine handover reads like two clients fighting over one address. Reports the backend's
typeverbatim rather than translating it: the guest's own vif is the one markedvif_ioemu(it is the one with an emulated counterpart), and a PV-only guest's would also be a plain
vif,so any friendlier label would be an inference in a log line. Drop this commit if you would rather
not carry it.
5.
dispatcher: retry a failed reconnect instead of abandoning a live vif.A connection attempt can fail transiently — a guest resetting mid-handshake leaves a revoked grant,
a stale event-channel port, or half-torn-down ring keys, none of which are
Xs_protocol.Error.Treating those as terminal abandoned a vif that still exists, and
wait_clientsonly re-adds a vifwhose directory is absent, so that client had no network until detach/reattach. Also distinguishes
the two spellings of "gone": a vanished frontend directory surfaces as
Xs_protocol.Error, abackend directory removed first as a raw
Xs_protocol.Enoent.6. Pin and hashsum.
This PR cannot go green on its own — please read before running CI
config.mlpinsmirage-net-xen ~min:"2.2.0", the release that carries the backend fixes. Untilthat release exists in opam,
make dependcannot resolve and CI fails at the build step, not atthe hash check. The mirage-net-xen PR has to land and be released first.
The
qubes-firewall.sha256in the last commit is a placeholder: it is the hash of a build madehere against the patched library source, not against a released 2.2.0 tarball, so it will not match
what CI produces even once the release exists. It needs regenerating (
./build-with.sh docker,copy
sha256sum dist/qubes-firewall.xen) once the pin resolves against the real release — happy topush that as a follow-up commit, or drop commit 6 entirely and let a maintainer do the pin.
Testing
Windows 10 22H2 HVM on Qubes OS 4.3, plus ~28 Linux client qubes on the same firewall.
The Windows guest attaches and reconnects across 20+ cold boots, and the reconnect path is also
driven directly by detaching and re-attaching its netvm while it runs: the vif disappears and a new
one arrives, and the firewall serves it again without a restart. Switching the guest to a different
netvm works too — it picks up the new gateway with no special handling.
The firewall has stayed up throughout: repeated guest reboots, guest resets mid-handshake, and live
vif removal are all routine now, where each of them previously took the whole unikernel down.