fix(firecracker): refuse a colliding tap name instead of silently sharing one - #201
Merged
Conversation
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
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.
Tap names are derived from a 14-bit hash of the instance id, so two instances can land on the same network slot. The documented consequence was that the second VM would fail to bring its tap up and crash-loop. That is not what happens:
TUNSETIFFon an existing interface name succeeds and reuses it. Two guests would end up sharing one tap and one host IP, with nothing reported, and tearing one VM down would delete the other's interface out from under a running workload.TapDevice::createnow refuses a name that already exists, turning silent corruption into a plain boot failure that names the interface and the remedy.Refusing alone would have made things worse.
TapDevice::deleteis best-effort — it gives up onEPERMand after exhausting itsEBUSYretries — so a teardown can leave an interface behind, and that leftover would then block every future boot hashing to the same slot: a rare silent collision traded for a permanent outage. So a boot first reclaims a tap that exists but belongs to no live instance.Ownership is established two ways, and a reclaim needs both to agree:
/procscan re-deriving each livefirecrackerprocess' tap from its--api-sockargument, which covers a running VM whose socket file was removed.Anything that cannot be inspected counts as in use. On a host with
hidepidthe reclaim simply does not happen and the operator gets the manual remedy — failing to reclaim costs one boot, deleting a live VM's interface breaks a running workload.Two limits worth stating. If the interface is still present after the reclaim attempt (
EPERM, noCAP_NET_ADMIN), the boot still fails; a warning now says so explicitly, sincecreate's message alone would read as a hash collision. And the existence check is not atomic withTUNSETIFF— Ring boots instances sequentially within a reconcile tick, so it is not reachable today, but it would need revisiting if boots ever run in parallel.Cloud Hypervisor is untouched: the VMM creates its own taps there, so Ring does not gate them. That remains a known limitation, now documented in
host_netalongside the Firecracker behaviour.Tests
772 unit tests pass (6 new);
cargo fmtandcargo clippy --all-targetsare clean. The new tests cover the refusal, the cross-deployment instance inventory, the ownership lookup that protects a live VM, and the cmdline parsing the/proccheck relies on. The tap creation path itself needsCAP_NET_ADMINand is only exercised by the e2e suite, which does not run in CI.