(4)-masquerade: find a pool without relying on disjoint prefixes by daniel-noland · Pull Request #1697 · githedgehog/dataplane

daniel-noland · 2026-08-06T01:43:08Z

Warning

AI assisted, not yet ready for external review by other humans.
Please do not spend review time on this yet. It is pushed to run CI and to keep
the stack visible, not to attract review. The dont-merge label stays on until
that changes.

This fixes no live bug, and is labelled refactor for that reason.

Looking a pool up walked back to the entry nearest below the private address and took it if
its prefix reached far enough. That is only right when the prefixes under one protocol and
pair of VPCs are disjoint. A prefix nested inside another starts nearer to an address while
covering less of it, so for an address of the wider prefix above the nested one the walk
stopped on the nested prefix, found it too short, and reported no pool at all. Addresses below
the nested prefix were served correctly, since the walk starts at the address and never reaches
a prefix beginning above it.

Three separate things rule that out for a validated configuration: VpcExpose::validate
normalizes a prefix set into disjoint prefixes, validate_expose_collisions rejects any
overlap between two masquerade exposes of a manifest, and check_peering_count refuses a
second peering between one pair of VPCs.

It is still worth not depending on them. Those guarantees live in another crate and nothing
near the lookup says the lookup rests on them, which is the distant reasoning
development/code/avoid-global-reasoning.md asks us to design out. Within this crate the
invariant is not enforced at all — the fuzz and unit harnesses build a PoolTable directly.
And the way it failed was misleading: it dropped the packet and logged that the allocator had
a bug, when the configuration was the unusual part.

Covered by a property test against a brute-force oracle over deliberately nesting-heavy inputs.
The oracle is an interval one -- nearest start, then narrowest -- rather than longest-prefix:
the generator produces intervals of any offset and length, most of them not CIDR-aligned, and
longest-prefix match is only defined on prefixes. The rule asserted agrees with longest-prefix on
everything the configuration layer can produce and is defined on what it cannot.

Also here, after review:

The lookup's doc corrected, folded down from (4)-masquerade: coverage, and the bugs it found #1700 so this PR does not make the two claims
it fixes. A nested prefix hid addresses above it, not either side; and overlaps are not
"reported when the table is built" -- add_entry warns only on two entries with identical
bounds, so a nesting or a partial overlap passes without a word.
The group-isolation test now crosses a boundary. Keys order by protocol, then source VPC,
then destination VPC, before the address, and the walk only looks back -- so the foreign group
it used sorted after the queried one and was excluded by the range bound before the guard ran.
The guard could be deleted outright and all five tests still passed. The test now puts a group
below the queried one on each of the three components in turn, and deleting the guard fails it.

Stack

Merge bottom to top. Each PR is based on the one above it in this list.

(1)-masquerade: allocate each public pair once #1696 — (1)-masquerade: allocator collision fixes
(2)-masquerade: port forwarding claims #1698 — (2)-masquerade: port forwarding reservation fixes
(3)-masquerade: the block-handover races, and honest concurrent oracles #1699 — (3)-masquerade: make the concurrent oracle honest
(4)-masquerade: find a pool without relying on disjoint prefixes #1697 — (4)-masquerade: find a pool without relying on disjoint prefixes ← you are here
(4)-masquerade: coverage, and the bugs it found #1700 — (5)-masquerade: allocator coverage, and the bugs it found

Every commit in the stack passes cargo nextest run on its own.

coderabbitai · 2026-08-06T01:43:15Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 08c4d757-8a23-4b48-bf19-daeeb1194c1f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

Refactors PoolTable lookup in the NAT masquerade allocator so pool selection no longer depends on private prefixes being disjoint. This makes lookups correct even when private prefixes are nested, by performing a backward scan that finds the best (closest-starting, then most-specific) covering entry within the same (protocol, src_vpc, dst_vpc) group.

Changes:

Update PoolTable::get to continue scanning past non-covering nested prefixes and to select the most specific covering prefix.
Add a bolero property test that checks lookups against a brute-force oracle over nesting-heavy inputs.
Add focused unit tests covering nested prefixes, deep nesting, and ensuring the scan doesn’t cross VPC groups.

+    /// Where several prefixes cover the address the narrowest wins, which is the longest-prefix
+    /// match the rest of the system uses. Overlapping private prefixes are an ambiguous
+    /// configuration rather than an expected one, and are reported when the table is built.


Applying a new masquerade config is not atomic from the data plane's point of view. The writer builds a fresh allocator, carries the surviving flows into it by re-reserving the address and port each one holds, and only then publishes it, while packet threads keep allocating from whichever allocator is currently published. Drive that with bolero as the outer loop, picking the public ranges and an op stream per thread, and the concurrency backend as the inner loop, exploring interleavings of that shape. Every lock and atomic the allocator uses comes from concurrency::sync, so the model checker sees the compare-exchange that claims a port block, the map of weak references to allocated blocks, the per-thread block hint, and the pool locks. Three properties are asserted: a published allocator never hands out an address and port carried over into it, no address and port is held by two flows drawn from the same allocator, and neither allocation nor reservation ever reports an internal bookkeeping error. The last one targets the standing FIXME in find_block_for_port, which wonders whether a block found non-free can be released before it is looked up; racing reservation against allocation is what would show it. Four thousand shapes under the shuttle portfolio did not, which is worth recording as a negative result rather than a proof. The record of live allocations is shared rather than per thread, since a collision between two threads is the interesting one, and an allocation is freed while that record is locked so no other thread can claim it before the allocator has released it. The suite goes through #[concurrency::model_test]. Under a model checker `just test` filters the run to test names containing the backend, because concurrency::sync types are then model-checker primitives and every other test in the workspace would fail spuriously outside a model-checked body. #[concurrency::test] earns that leaf but also wraps the body in stress(), which is the wrong shape when bolero has to be the outer loop; model_test emits the same leaf and leaves the body verbatim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…lock CI caught this before we did: sanitize/fuzz/thread on #1699 ran for six hours and was killed with the nat test binary still alive, one test short of the suite. Locally the same test wedges within about ten runs, and 3000 runs pass with this change. The pool keeps weak references to the addresses in use; the strong ones belong to the port blocks handed out from each address. Upgrading one while holding the pool lock is safe only until the last flow on that address ends somewhere else, at which point the upgrade here is the only strong reference left and letting it go runs AllocatedIp::drop on this thread. That drop asks the pool for its write lock, which this thread is already holding. The core stops, for good. Three places did it, all reached by IpAllocator::allocate, which is the path every new flow takes: * cleanup, which upgrades each entry to see whether it still resolves, under the write lock. This is the one that hangs: it runs on every allocation, and the temporary upgrade is dropped immediately. * reuse_allocated_ip, under the read lock, for each address it passes over. * reserve_from_pool, under the write lock, for each address that is not the one being carried over. Each now keeps what it upgraded until the guard is gone and releases it after. Confirmed by intervention rather than by reading: fixing only reuse_allocated_ip left it hanging at iteration 25, and fixing cleanup took it to 3000 clean. Pre-existing: cleanup is unchanged from main, and the test that exposes it is on main too. It hid because the window is small and needs a flow ending on one thread while another allocates. sanitize/fuzz/thread found it because it runs the whole suite on real threads for long enough. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The lock-lifetime fix covered the three allocation paths that upgrade a weak address reference under the pool guard, and missed a fourth: the Display impl. `IpAllocator::fmt` takes the read guard and hands it to `NatPool::fmt`, which upgrades every weak reference in the in-use list to print it. An address whose last block is released just then leaves the upgrade taken for printing as the only strong reference, and dropping it runs `AllocatedIp::drop` on the printing thread, which takes that same lock for writing. Same self-deadlock, reached from the management side rather than the packet path: `NatAllocator` is a `CliSource`, so the table is formatted on its own thread while packet threads keep ending flows. Answered the same way as the other three -- every address is upgraded into a vector that outlives the guard, so nothing printed can be the last reference, and the vector is released once the guard is gone. Shuttle finds it in one execution and names it: "tried to acquire a RwLock it already holds". The test added here is that race; it deadlocks without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Running the suite exercises the bolero properties through the random driver, which samples blindly and only runs briefly. libfuzzer mutates towards inputs that reach new code, and reaches a different order of magnitude: a property managing a few thousand cases per second under `just test` does several hundred thousand per minute here. cargo-bolero already ships in the nix shell, already builds with the fuzz profile, and already links AddressSanitizer, so a plain `just fuzz` is an asan campaign and this is only the two recipes plus the documentation the testing guide said was still to come. `just fuzz-list` names the targets and `just fuzz <target> [duration]` runs one, forwarding anything further to `cargo bolero test`. Sanitizer choice reuses the justfile's existing `sanitize` variable rather than a positional argument, so it composes the same way it does for `just test` and does not disturb the arguments passed through. `sanitize=thread` also rebuilds std, because thread instrumentation changes the ABI and a std left uninstrumented fails the build on a mismatch against `core`; address needs no such thing, and skipping the std rebuild keeps the common case quick. The recipe passes --rustc-bootstrap: libfuzzer wants a nightly compiler for its sanitizer coverage flags and the pinned toolchain is stable. Nothing needs to be committed afterwards, since the corpus lands in a `__fuzz__` directory that is already gitignored. Running a campaign across several cores, which is the cheapest way to reach deeper into a property, leaves one `fuzz-<n>.log` per worker in the directory it was run from rather than under `__fuzz__`. Those are gitignored too, and the guide says both that `-j` is there and where its logs go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

PoolSet::allocate tries an expose's regions in turn, and treated every error as a reason to try the next one. That is right for a region that is full, and wrong for everything else: an allocator reporting that its own bookkeeping is inconsistent would be buried under a later region's success, or replaced by a later region's NoFreeIp, and the caller would never learn that anything was wrong. Fall through only on exhaustion, and return anything else straight away. The classification lives on AllocatorError as is_exhaustion, next to the DoneReason conversion that already draws the same line by mapping exactly those three variants to NatOutOfResources. Its match is exhaustive, so a new variant has to be classified rather than silently defaulting. This matters most to the model-check suite, which panics when allocation reports an internal issue: that assertion targets the standing FIXME in find_block_for_port, and until now it could only see the error if the failing region happened to be the last one tried. In practice the two errors this newly propagates are both unreachable from configuration today, which is also why the test added here covers the fallback rather than the propagation: a region can be exhausted on demand by reserving its ports, but an allocator cannot be made to report an internal issue without a fault-injection hook it does not have. The test therefore pins that exhaustion still falls through, which is the behaviour this change could have broken. The same rule applies one level down, and did not hold there. Drawing a fresh address is what to do when the addresses already in hand have no room, and only then; `reuse_allocated_ip` distinguishes the two, and the caller took only its `Ok` and threw the rest away. An error about the allocator on the reuse path was therefore buried under whatever the fresh address returned -- and since the concurrent suite asserts that no allocation ever reports `InternalIssue`, that oracle was blind through exactly this path. Injecting one there returns `Ok` before this change and the error after it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

map_address turns an IPv6 address into its index in the pool bitmap, and panicked on the two ways that can fail: an address below everything in the mapping, and one too far above it to fit a u32. The second is reachable. A region may hold more addresses than a u32 can index, and NatPool::for_range deliberately keeps the first 2^32 of them rather than refusing to build the pool, so an address inside the region can still be one the bitmap cannot name. PoolSet::reserve only checks that some region contains the address, which is the untruncated range, so it hands such an address straight to the bitmap. Getting there needs a flow carried across a config change: it presents the address it already holds, and the region it falls in may have grown downwards underneath it, putting it further from the start than it was before. The panic would then land in the middle of applying a config, taking out the writer rather than the flow. Return NoPoolFound instead, which is what PoolSet::reserve already reports for an address no region covers, and which says the same thing here: this pool does not serve that address, so the flow cannot be carried over and is dropped like any other that cannot be. InternalIssue would have been wrong, both because configuration rather than a bug gets you here, and because the model-check suite treats it as an assertion failure. Deallocation has nowhere to report an error, since it runs while an allocation is being dropped, so it logs and leaves the address marked in use rather than freeing the wrong one. The tests are the first to exercise an IPv6 pool at all. Full IPv6 coverage of the property suites is still missing and wants doing separately; these cover the mapping this commit touches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The record of live allocations is written just after the allocator hands a pair out, not as part of it. That keeps the threads racing on the allocator's own locks rather than on the record's mutex, and it leaves a hole once an allocation can be freed: if two threads are wrongly given the same pair and the first releases it before the second records it, the second insertion succeeds and the duplicate is never seen. No record kept at those two points can close that hole. The interleaving is indistinguishable from one thread legitimately reusing what another gave back, which is why the comment claiming the collision is caught either way was wrong. What can be done is to remove the ambiguity. Roughly half the generated shapes now hold every allocation for the length of the run: nothing is released, the record only grows, and a duplicate is caught with certainty. The rest still free as they go, since deallocation is worth exercising, and still catch every duplicate whose holders overlap in the record. Packet threads therefore hand back what they are still holding instead of releasing it when their ops run out, which also removes a smaller version of the same problem: a thread that finished early used to free addresses while the others were still allocating. Nothing else can legitimately be given a pair that is still held, so keeping them costs no false positives and the end-of-run release it replaces is not needed. Verified by mutation rather than by argument alone: dropping the line in allocate_port_from_bitmap that marks a port used makes the allocator hand the same pair out repeatedly, and both this suite and the pool property suite fail on it. Closing the remaining gap outright would take recording a pair as part of handing it out, which means instrumenting the allocator itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

AllocatedPort frees its port when it is dropped, and it was also Clone, so every copy freed the same port. Dropping a copy released a pair the original still held, and the allocator would then hand that pair to a second flow -- the collision this series exists to prevent, since the reverse flow key cannot tell the two apart. let original = pool.allocate(false)?; drop(original.clone()); pool.reserve(original.ip(), original.port())?; // succeeded Harmless while freeing was broken, because a drop that does nothing is harmless to repeat. The bitmap fix at the bottom of this PR is what makes it bite, so it is fixed in the same PR rather than left for a later one to discover. Clone comes off AllocatedPort, and off Allocation and MasqueradeState with it. Nothing in the data plane wanted it: the library builds with all three non-Clone and no other change. The only caller was a test helper cloning a whole live MasqueradeState to read two fields off it -- itself a second owner of a live allocation -- which now borrows under the lock. Preferred over keeping Clone and hiding the deallocation behind a shared lease. An allocation is a lease exactly one thing holds; making that unrepresentable is worth more than making it correct by reference count. Two smaller doors onto the same room, closed here as well. `NatPool` no longer derives `Clone`: nothing cloned a whole pool, and a clone would be two pools over one range of public space, which is this PR's bug one layer down from the exposes. And `AllocatedPort::drop` says something when a port cannot be given back rather than discarding the result -- still no panic on a drop path, but a port that refuses to be freed means the bitmap has stopped describing what is in use, which is exactly what went unnoticed before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

A masquerade flow expires against the wall clock, while a test refreshes it by doing work: sending a packet. Under miri or qemu-user that work runs something like two orders of magnitude slower, so the gap between one packet and the next stops being a fraction of a flow's life and becomes several times it. A flow a native run keeps comfortably alive is one an emulated run finds long dead. That is what has been failing check/miri/powerpc64 on this stack. test_masquerade_reconfig_two_vpcs_sharing_a_private_prefix never answers its SYNs, so its flows carry the one-way timeout of five seconds, and it does two packets and four assertions between the last refresh and the count. Natively that is about a second. Under miri it is well past five, all four flows are gone, and active_len() is 0 where the test wants 4. Nothing to do with the allocator. The test arrives in the second PR of this stack, which is the whole of why the job passes on the first and fails on every one after it: the first does not have the test. Not a matter of which seed a run drew, as I first supposed. The constants and the sleeps against them are older than the stack, and the same cliff is under the other masquerade tests that use them. test_masquerade_reconfig_keep_flow finished within a second of the one that failed, so this was going to spread. Confirmed by reproducing it natively, with no emulator involved: sleeping six seconds rather than one in that test gives the identical failure, Some(0) against Some(4). With the timeouts stretched, the same six-second stall passes. `emulated` is set only by the miri and qemu-user paths, so a real data plane is unaffected. The whole nat suite passes with the cfg forced on, which also shows the three tests that assert a flow count of zero are driven by invalidation rather than by a timeout firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Keying the pool table by source VPC made a private address mean something only inside its own VPC. The allocator tests cover that directly, but nothing drove it through the pipeline, where the address has to survive being looked up on the packet path and again when the configuration is applied. Two VPCs both using 1.1.0.0/16 and both masquerading towards a third, onto public ranges of their own. Each is translated onto the range its own expose declares, and both keep that address across a configuration change rather than one of them landing in the other's pool. The change applied here is an identical configuration, which the writer answers by keeping the allocator it has and only advancing the flows' generation. That is the common case and worth covering, but it is not the carry-over path: re-reservation is reached only when the configuration really differs, and a later commit in this series covers that. The flow-creating packet does not carry the flow it creates, so the assertions are made on a follow-up packet, which is also what shows the generation advancing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

A public address could carry only one claimed port range. Port forwarding names a public range and a port range per expose, and nothing stops two exposes naming the same address with different ports, so the second claim replaced the first and masquerade went on handing out ports that port forwarding had statically mapped elsewhere. The single range was baked in three times over: the claims were collected into a map keyed by address range, where inserting twice under one key overwrote; the pool then resolved one range per address out of that map, since the lookup returns a single entry; and the port allocator stored one range. Each had to change together, which is why the tests covering this were committed ignored rather than fixed piecemeal. Carry the claims as a list instead, in a type of their own. A list rather than a map because claims may be made on overlapping address ranges and every claim covering an address applies, not merely the innermost or the last recorded, and reserving a port in a block bitmap is an idempotent OR, so overlapping claims need no merging. Two things needed more than a mechanical change: Skipping a fully claimed port block used to ask whether one claim covered it. Several claims may cover a block between them while no single one of them does, so this now walks the union. The old special case for port 0, which is never handed out for TCP or UDP and so need not be covered for the first block to be useless, is kept. Claims are clipped to a block before they reach its bitmap, which indexes ports modulo 256 and silently reserves nothing when handed a range crossing its end. That clipping already existed for the single range; it now happens per claim, and also on the path where an address enters the pool through a reservation. That path passed no claims at all, so a flow carried across a config change could bring an address in unencumbered and let masquerade hand out the ports port forwarding held on it. Both previously ignored tests now run: the pools honour claims expressed in public space. Computing those claims from the configuration is the separate defect noted on find_masquerade_portfw_overlap, and is next. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The ports masquerade had to keep off were worked out by intersecting the private prefixes of a port-forwarding expose with the private prefixes of a masquerade expose. The pools are then asked about them by public address, because a public address and port is what an allocation is, so the claims described a space nothing ever looks in and effectively nothing was reserved. A port-forwarding expose is validated down to one prefix on each side, of equal size and with a port range on both, so its public claim needs no offset arithmetic: it is exactly its as_range. Record that instead. Claims are also collected per peer VPC now, rather than per manifest. The public space towards a peer is shared between every VPC masquerading onto it, which is what the region decomposition already exists for, and return traffic carries nothing that says which VPC it belongs to. A claim therefore binds the space rather than the expose that declared it: one VPC's port forwarding has to be honoured by another VPC's pools, and a peering that port-forwards without masquerading no longer has its claims dropped for want of a masquerade expose to hang them on. That makes the claims a property of the public space rather than of an expose, so they move off PoolSpec and are passed to the pool builder once per peer VPC and protocol. The masquerade expose's own protocol no longer narrows a claim. Pools are built for TCP, UDP and ICMP whatever an expose declares, so a TCP claim belongs in the TCP pool regardless, and intersecting the two protocols only dropped claims that were still live. Verified by mutation: putting the private prefixes back makes the new end-to-end test hand out 10.1.0.0:1024, the first port the forwarding expose has taken. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Allocation drew the lowest free address from a region and, if no port could be had from it, handed it straight back and gave up. The same address was lowest next time, so the pool served nothing at all for as long as it stayed there. One public address whose every port is claimed by port forwarding therefore took a whole region out of service, and the claims only had to land on the lowest address in it: the same claim one address higher was harmless, because allocation never reached it. This became reachable when claims started being computed in the public space. Before that they described private addresses the pools are never asked about, so nothing was ever reserved and no address could be fully claimed. The configuration it needs is allowed: validation permits a masquerade expose and a port-forwarding expose to overlap, which is the whole reason masquerade keeps off the ports port forwarding has taken. Draw another address instead. An address that comes fresh out of the pool and yields no port is one whose every port is spoken for, which does not change while the pool lives, so it is taken out rather than handed back and the walk moves on. The walk is bounded, because a data plane cannot search without a bound on the packet path. The bound costs nothing when the first address serves, and since a useless address is taken out as it is found, a long claimed run is worked through over successive packets rather than being walked again by each of them. The tests reach exhaustion by claiming every port of an address, which is what makes it testable at all: allocation stays on one address for 64k ports, so a handful of allocations never leaves the first address of the first region. That was the substance of a review comment on the property tests, and it hid this. They now cover an address being passed over, a region with nothing left reporting exhaustion, every address of a region being reachable in turn, and an expose falling back to shared space once the space it has to itself is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

A block port forwarding had claimed in full was skipped when picking one to allocate from, and the skip marked it non-free without taking it off the count of blocks still usable. The count only ever went down when a block was actually allocated, so it stood permanently higher than the truth. That count is what decides whether an address is worth trying. An address whose remaining blocks were all claimed still reported room, so reuse tried it, and the attempt failed with "no port block" rather than "no free port" -- and the walk over addresses already in hand returned on that error instead of moving to the next one. Every allocation from then on drew a fresh address while the addresses in hand sat with tens of thousands of free ports. A region of four addresses with one claim on one of them ran out after 259 allocations, where it holds room for over a quarter of a million. Decide which blocks are unusable once, when the allocator is built: the well-known range, as before, and now also the blocks claimed in full. Then count the usable ones from the blocks themselves rather than computing what the count ought to be, so the two cannot disagree. Picking a block becomes just claiming the first free flag, which takes the claims off the allocation path entirely. Reuse also moves on now when an address turns out to have nothing left, whichever way it says so, rather than giving up on the whole walk. Either change alone is enough for the case above, and the test passes with either reverted; it pins the outcome rather than the mechanism. The second is worth keeping regardless, since an address can be emptied by another thread between being judged worth trying and being tried. Two more ways the count could stop describing the blocks, found by review of this PR. Giving a block back raised its flag before adding to the count, so a claimant winning the flag in between subtracted from a count that was still short -- on a `u16`, past zero to 65535, which says an address has room it does not have. The count is given back first now, so the transient error is one too many rather than a wrap. And a block whose construction failed after its flag was taken and the count lowered gave neither back: no `Arc` existed, so no `Drop` was coming, and the block was claimed by nobody for the life of the allocator. Both paths hand it back. The well-known-range refusal in `reserve_port` also no longer depends on knowing what `setup.rs` does. The range is a port-number convention and says nothing about an ICMP identifier; identifiers cannot reach it today because ICMP pools are built with the exclusion off, but nothing here said so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…g error Reserving a port looks the block holding it up, and reads a block that is not free as one that is allocated and therefore present in the list of allocated blocks. Absent from that list, it concluded the allocator's own bookkeeping was broken and returned InternalIssue. That stopped being true once a block port forwarding has claimed in full could be marked non-free without ever being allocated. Such a block never joins the list, so its absence says nothing. A configuration reaches this: keep a masquerade expose, add port forwarding covering the whole 256-port block around a port some flow holds, and carry that flow over. The flow is dropped, which is right, but a legal conflict between two parts of a valid configuration is logged as a bug in the allocator, and reported upwards as InternalFailure rather than as a NAT failure. Answer as a claim on part of the same block already does, where the block stays allocatable and its own bitmap refuses the port. How much of a block an operator happened to claim is not something a caller should be able to tell apart, and it is certainly not the difference between a policy conflict and broken bookkeeping. The condition mirrors the one that marks blocks unusable when the allocator is built, and has to keep mirroring it, so it is written as a single predicate next to the lookup that needs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…e pool Allocation draws the lowest free address, so a port-forwarded prefix at the bottom of a region is what it meets first. Every one of those addresses was drawn, found to have no port to give, and taken back out -- and an allocation may work through only MAX_ADDRESSES_PER_ALLOCATION of them before it gives up and the packet is dropped. A run of N therefore cost floor(N/8) dropped packets: sixteen for a claimed /25, measured, on flows the region had tens of thousands of ports waiting for. Retirement kept it from being worse than that, since the next packet carried on where the last stopped, but a fresh allocator starts with a fresh pool, so the whole run was rediscovered after every config change, per protocol. Which addresses those are is known when the pool is built. Sweep the claims rather than the addresses -- a region may hold billions of the latter and only as many of the former as there are port-forwarding exposes towards one peer -- cutting where a claim begins or just past where one ends, since coverage cannot change anywhere else. One address then decides each stretch, and the stretches that can serve nothing are taken out of the bitmap before anything is allocated. Whether an address can serve is asked through the same predicate the port allocator uses to rule a block out, applied to all 256 of them, rather than by a second definition of the same thing written over the port space. The two drifting apart would mean either dropping an address that still had a block to give, which is capacity silently thrown away, or keeping one that had none, which is the walk this removes. The exclusion needs somewhere to live that a drop cannot undo. Reserving reaches addresses allocation never touches: a flow carried across a config change presents the address it holds, and the pool takes it into use to try to give the port back. Where the new configuration has claimed that address the reservation fails, correctly -- but the address has been through the pool by then, and deallocate_from_pool would hand it to the bitmap on the way out, undoing the exclusion on the first config change that needed it. Unusable offsets are therefore tracked apart from free ones, and consulted before an address is given back. That closes the same hole for retire_ip, where it was latent: an address retired for having nothing to give could be put back by a carry-over that failed on it. The bound stays, for the case configuration cannot produce: an address emptied by another thread between being drawn and being drawn upon. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The property checked what the sweep reports against `every_block_is_unusable` applied per address -- the same function the sweep calls. It verified the cutting into intervals, which is worth verifying, but it could say nothing about the predicate underneath, because it was comparing that predicate with itself. Mutation testing showed it: truncating the block loop, and excusing one block from being claimed, both left it green. It now decides the same question a different way, walking the claims over the port space to ask whether they cover it end to end, with no notion of a block anywhere in it. Both mutations fail against that, as does the one that stops excluding addresses altogether. The generator also draws port ranges in three shapes rather than two, the new one being a claim that stops exactly on a block boundary. Left to chance a range that ends on a multiple of 256 essentially never appears, and that is the shape the block-level and address-level answers can disagree on. Comparing the production code against itself is the risk that comes with routing both answers through one predicate so they cannot drift. The answer is not to give up the single definition, it is to test it against something written independently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…n a valid config The fixture put the masquerade expose in VPC-1's peering and the port-forwarding expose in VPC-2's, both onto one public address towards VPC-3. Production rejects that: config validation builds each VPC a route table from its peerings, and VPC-3 cannot route the shared address to one peering when the exposes come from two -- masquerade with masquerade may overlap there, masquerade with port forwarding may not. The fixture passed validation only because building a VpcTable by hand skips collecting peerings into the peer, so VPC-3's route table was empty and the check never ran. Both exposes now sit in one manifest, which is the shape validation accepts (validate_expose_collisions allows the overlap within a manifest, each mode implying a direction) and the shape the end-to-end reconfig test already uses. The test over the fixture is unchanged and still bites: the first port handed out is 1031, and without the claims it would be 1024. Cross-VPC claim sharing in gather_exposes stays, and this is worth being clear about: with the overlap rejected across VPCs, no validated configuration currently reaches it. It is defence of the same kind as the pool lookup that stopped relying on disjoint prefixes -- the guarantee lives in another crate, and nothing near the pools says they rest on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The property says a claim binds the public space it names whoever made it, and the pool is built that way: the claims of every expose are unioned and applied to every region over that space. The oracle did not check it that way. It skipped a claim unless the expose that made it also declared the address the allocation came from, so a claim made through one expose was never checked against an allocation made through another -- the one case the property exists to state. Building the pool from only each expose's own claims leaves the property passing over ten thousand inputs before this, and fails on the second input after it. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The pool-level half of the freeing fix, which now lands at the bottom of the stack so that everything built on top can rely on it. Dropping every allocation at once frees whole blocks, and a block is rebuilt from scratch whatever its bitmap said, so a test that does that passes whether or not an individual port is ever returned. Only a port given back while its block stays alive shows whether freeing works, and that is the ordinary case: one flow ending while its neighbours carry on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The ReserveExisting op reserved a survivor's pair on a published allocator and asserted only that the error was not InternalIssue. Two problems. It matched on Err in a let-chain, so a reservation that succeeded produced a temporary that dropped at the end of the statement. That releases the port the published generation is holding for the survivor. Had a bug ever let a pair be reserved twice, the op would not have caught it, and would have corrupted the state the other two oracles rest on while failing to. And succeeding is itself the interesting outcome. The writer re-reserves every survivor before publishing, so where a pair was carried over, a second reservation has to be refused: the same rule as for allocation, on the path a config change actually takes. That is now asserted. Where it was not carried over the pair is genuinely free, and a reservation that succeeds is recorded and held for the length of the run like any other allocation, rather than being handed back while the other threads work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

…at carried nothing Published::build accepted any carry-over failure short of InternalIssue, on the comment that the address may no longer be served or another survivor may hold the pair. Neither can happen: every generation is built from the same specs, so the address is still served; the survivors are distinct pairs; and the allocator is fresh, so nothing else holds them. A regression that made carry-over fail would have gone unseen, and every property about carried pairs would have passed vacuously over generations that quietly carried nothing. A survivor that fails to carry is a failure now. Being strict there also settles what ReserveExisting is: every survivor is always carried, so in a correct allocator the reservation is always refused, and the success arm is an oracle for a double-reservation bug rather than a covered path. Its comment now says so. The module doc claimed reserving concurrently with allocating is what would show the standing find_block_for_port FIXME, the block released between the CAS and the lookup. It is not, and the doc now explains why: reservations target survivors, and a survivor's block is pinned for the whole generation by the reservation Published holds, so it cannot disappear mid-lookup. Reaching that interleaving takes generations whose specs differ, so that a pair stops being carried and its block can empty while another thread reserves it. That is the suite's next extension, recorded rather than implied to exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

A block is given back in two steps a reader can fall between: the free flag is stored true, and the weak entry in the list of allocated blocks expires when the last Arc to it goes. A reservation reads the flag and then searches that list, so it could find the block neither free nor allocated and returned InternalIssue -- the allocator declaring its own bookkeeping broken -- for a moment in which nothing is wrong. The lookup now starts over instead. The port the reservation wants is free by then, so the next attempt claims the block and allocates it. The loop is bounded because a thread that keeps allocating and releasing this same block could otherwise hold a reserver in the window indefinitely; on exhaustion the answer is a failed reservation, which costs one flow, rather than InternalIssue, which the caller reads as the allocator being unfit and which the concurrency model treats as fatal. This carried a FIXME wondering whether the window was reachable and noting it had not been seen in shuttle. It is reachable, and shuttle finds it in one execution: the test added here allocates a pair, races a reservation of it against dropping its only holder, and accepts either legitimate answer -- refused while held, granted once released. It fails on the first schedule without the retry. Reaching it needs neither a config change nor generations whose specs differ, which is what the module doc claimed; one pool and one generation will do. Production meets it through the late flow nf.rs handles, where a packet that allocated from the previous allocator installs its flow after a new one was published: that pair is not among the writer's pinned survivors, so the block behind it may be emptying as it re-reserves. The bug predates this stack. It is fixed here because this is the PR that makes the concurrent oracle honest, and the claim it corrects is one this PR's own documentation made. A third instance of the same shape, found by review of this PR and fixed here with it. Tidying a dead entry out of the list of allocated blocks looks the entry up and drops it under separate locks, so another task can claim the freed block and list it at that index in between -- and the drop then deletes an entry for a block in use. Nothing is handed out twice, so this is availability rather than isolation: the orphaned block stays claimed by its holder while the allocator no longer knows of it, so reservations into it are refused and its free ports stop counting towards the address having room. Re-checking under the write lock is the answer here, rather than a retry, since the caller has a lock to take anyway. `search_for_block` upgraded twice for the same reason -- once to test the block, once to return it -- and could report a block absent because it died between the two. It keeps the first upgrade now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Looking a pool up walked back to the entry nearest below the private address and took it if its prefix reached far enough. That is only right when the prefixes under one protocol and pair of VPCs are disjoint. A prefix nested inside another starts nearer to an address while covering less of it, so for an address of the wider prefix above the nested one the walk stopped on the nested prefix, found it too short, and reported no pool at all. Addresses below it were served correctly: the walk starts at the address and never reaches a prefix that begins above it. This is not reachable from a validated configuration, and this commit fixes no live bug. Three separate things rule it out: VpcExpose::validate normalizes a set of prefixes into disjoint ones, so one expose cannot hold a nesting pair; validate_expose_collisions rejects any overlap between two masquerade exposes of a manifest, nesting included; and check_peering_count refuses a second peering between one pair of VPCs, so a key's entries all come from a single manifest. It is worth not depending on that. The three guarantees live in another crate and nothing near the lookup says the lookup rests on them, which is the sort of distant reasoning the development guide asks us to design out (development/code/avoid-global-reasoning.md, "code should be modular"). Within this crate the invariant is not enforced at all: the fuzz and unit harnesses build a PoolTable directly, so a nested pair is one line away. And the way it failed was misleading, dropping the packet and logging that the allocator had a bug, when the configuration was the unusual part. The walk now continues past an entry that does not cover the address, and stops once nothing further back can be a better match. Where more than one prefix covers the address the narrowest serves it, which is the longest-prefix match used everywhere else. The cost is bounded by the entries of one protocol and pair of VPCs, and only paid on the first packet of a flow. With disjoint prefixes it stops after two steps, as before. The case that now walks a whole group is an address no prefix covers, which is itself supposed to be unreachable. Covered by a property test that checks every address in a window against a brute-force longest-prefix oracle, over sets drawn narrowly enough that nesting is the common case. The walk also refuses to leave the run of keys sharing its protocol and pair of VPCs. Keys order by those three before the address, and the walk only ever looks back, so a group sorting *after* the queried one is cut off by the range bound and never reaches that guard: the test for it puts one group below the queried one on each of the three components in turn, which is what makes deleting the guard fail. Placed above, as it first was, the guard could be deleted outright and every test here still passed. The generated property is an interval oracle, not a longest-prefix one. The generator produces intervals of any offset and length, most of them not CIDR-aligned, and longest-prefix match is only defined on prefixes. The rule asserted -- nearest start, then narrowest -- agrees with it on the inputs the configuration layer can produce and is defined on the ones it cannot. The generated test marks each entry with both its bounds rather than only its end. Two entries ending together were carrying the same marker however far apart they started, so the oracle could not name which of them a lookup had landed on -- and "nearest start" is half the rule it checks. No regression is known to slip through: the walk stops at the first start below the one it has settled on, so two such entries are never both considered, and a mutation that removes that stop is caught on the inputs where ends differ. An oracle that compares entries should be able to tell them apart regardless. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Five of them, each keeping its reason for existing and losing the restatement around it: the concurrent record's description of what it cannot catch, the two pool properties that explain why counting is not enough, the IPv6 span test, and two end-to-end tests whose second paragraph repeated the first. The correction to `PoolTable::get`'s doc that this commit used to carry has moved down to #1697, where the claims it corrects are made. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland · 2026-08-07T01:51:47Z

Folded into #1696 and closed as part of a review-friendliness pass over the stack.

This was a one-commit PR whose content — the pool lookup that no longer relies on private prefixes being disjoint — belongs next to the commits that build the pool table in the first place (Identify NAT pools by their public range, Key NAT pools by source VPC as well, Allocate from disjoint regions of the public space). It now sits directly after them in #1696 as refactor(masquerade): Find a pool without relying on disjoint prefixes, unchanged apart from the review fixes it already carried.

The stack is now four PRs: #1696 → #1698 → #1699 → #1700.

daniel-noland self-assigned this Aug 6, 2026

daniel-noland added dont-merge Do not merge this Pull Request ci:+vlab Enable VLAB tests labels Aug 6, 2026

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 26373ed to 206dbaa Compare August 6, 2026 02:27

daniel-noland changed the base branch from main to pr/daniel-noland/port-bitmap-free August 6, 2026 02:28

daniel-noland changed the title ~~pool table lookup~~ masquerade: find a pool without relying on disjoint prefixes Aug 6, 2026

This was referenced Aug 6, 2026

(1)-masquerade: allocate each public pair once #1696

Open

(2)-masquerade: port forwarding claims #1698

Draft

(3)-masquerade: the block-handover races, and honest concurrent oracles #1699

Draft

(4)-masquerade: coverage, and the bugs it found #1700

Draft

daniel-noland changed the title ~~masquerade: find a pool without relying on disjoint prefixes~~ (4)-masquerade: find a pool without relying on disjoint prefixes Aug 6, 2026

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 206dbaa to 687a54e Compare August 6, 2026 04:31

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch from e862f7f to 6cc4f54 Compare August 6, 2026 04:35

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from 687a54e to a348ddd Compare August 6, 2026 04:36

daniel-noland requested a review from Copilot August 6, 2026 05:58

Copilot started reviewing on behalf of daniel-noland August 6, 2026 05:59 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from a348ddd to bf7198e Compare August 6, 2026 20:45

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch from 74bbea2 to 3e63b8e Compare August 6, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from bf7198e to dd99d23 Compare August 6, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch from 3e63b8e to 2b807d5 Compare August 6, 2026 21:47

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch 2 times, most recently from 03071be to da2c429 Compare August 6, 2026 22:29

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch 2 times, most recently from 12afe1d to f86b776 Compare August 6, 2026 22:33

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch 2 times, most recently from 0b06b6e to a23149c Compare August 6, 2026 22:49

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch from f86b776 to 12c53b7 Compare August 6, 2026 22:49

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from a23149c to a1348f9 Compare August 6, 2026 23:11

daniel-noland and others added 24 commits August 6, 2026 19:21

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch from 5010f75 to f60f8f0 Compare August 7, 2026 01:30

daniel-noland force-pushed the pr/daniel-noland/pool-table-lookup branch from dcfa3a1 to 5a47ebc Compare August 7, 2026 01:30

daniel-noland force-pushed the pr/daniel-noland/port-bitmap-free branch from f60f8f0 to 6a43add Compare August 7, 2026 01:50

daniel-noland closed this Aug 7, 2026

Navigation Menu

Uh oh!

(4)-masquerade: find a pool without relying on disjoint prefixes - #1697

(4)-masquerade: find a pool without relying on disjoint prefixes#1697
daniel-noland wants to merge 24 commits into
pr/daniel-noland/port-bitmap-freefrom
pr/daniel-noland/pool-table-lookup

daniel-noland commented Aug 6, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

daniel-noland commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

daniel-noland commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stack

Uh oh!

coderabbitai Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

daniel-noland commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniel-noland commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading