(1)-masquerade: allocate each public pair once by daniel-noland · Pull Request #1696 · githedgehog/dataplane

daniel-noland · 2026-08-05T18:56:22Z

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.

Masquerade hands out a public (address, port) pair, and return traffic is matched on a
reverse flow key built from that pair, the remote endpoint and the peer VPC. Nothing in that
key says which VPC the traffic came from, so two flows that collide on it are
indistinguishable and one tenant's return traffic can be delivered into another's VPC.

This branch fixes three ways the allocator could produce such a collision, and builds the
test machinery to show it does not.

Pools identified by their public range. Exposes sharing a public range each got their
own allocator, so each believed it owned the whole range.
Pools keyed by source VPC. A private address only means something inside its own VPC.
Two VPCs using the same private space produced one key, and the second silently replaced
the first.
Allocation from disjoint regions. Public ranges may overlap only partially, where
neither contains the other. The public space is now cut at every point where the set of
exposes covering it changes, giving maximal intervals with one allocator each.

It also carries, as its first commit, the set_bitmap_value fix that used to sit in #1699:
freeing a port was an OR with zero and never freed, and the guard against reserving a port already
taken compared the wrong two values, so a pair another flow held could be handed out again. That is
a fourth collision vector, and it belongs here because everything above depends on it -- in
particular the concurrent model checker, which cannot assert that a held pair is refused until the
guard works. Its pool-level test needs machinery introduced later and stays in #1699.

Fixing the bitmap turned a dormant hazard into a live one, so that is fixed here too: an
AllocatedPort frees its port on drop and was also Clone, so every copy freed the same port.
Dropping a copy released a pair its original still held, and the allocator would hand that pair to
a second flow. Dormant only because a drop that does nothing is harmless to repeat. Clone comes
off the allocation, and off Allocation and MasqueradeState with it -- the data plane never
wanted it, and the one caller was a test helper cloning live state to read two fields.

Then the testing those rest on: a bolero x model-checker suite over a config change, just
recipes for real libfuzzer campaigns, and two follow-on fixes found while writing them (a
non-exhaustion error being masked by region fallback, and a panic on an IPv6 address a pool
cannot index).

Every commit passes cargo nextest run on its own.

The allocator can deadlock against its own pool lock

Moved here from the top of the stack. It predates the stack and depends on nothing in it, and at
the top it left every PR below it running with the hang -- which is not hypothetical: it wedged a
thread-sanitizer CI job for six hours before it was understood.

The pool holds weak references to the addresses in use; the strong ones belong to the blocks handed
out from each. Upgrading one under the pool guard and then letting it go can run
AllocatedIp::drop on the same thread, and that takes the same lock for writing. Another thread
ending the last flow on an address at that moment is all it takes. Four sites did it:
reuse_allocated_ip, cleanup, reserve_from_pool, and -- found by review of this PR, after the
first three were fixed -- IpAllocator::fmt, which reaches it from the management side, since
NatAllocator is a CliSource and the table is formatted on its own thread.

Each now keeps every upgrade alive until the guard is gone. Two Shuttle regressions come with it;
both deadlock in a single execution without the fix, and Shuttle names it directly: "tried to
acquire a RwLock it already holds".

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: allocate each public pair once ← you are here
(2)-masquerade: port forwarding claims #1698 — (2)-masquerade: port forwarding claims
(3)-masquerade: the block-handover races, and honest concurrent oracles #1699 — (3)-masquerade: the block-handover races, and honest concurrent oracles
(4)-masquerade: coverage, and the bugs it found #1700 — (4)-masquerade: coverage, and the bugs it found

#1697 was folded into #1696 and closed; its one commit belonged next to the other pool-table work.

Every commit in the stack builds and passes cargo nextest run and cargo clippy --all-targets
on its own.

coderabbitai · 2026-08-05T18:56:32Z

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: 9eab0332-1ad9-433a-adc1-a63526bc7962

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 the masquerade NAT address/port allocator to correctly distinguish pools by (source VPC, destination VPC, private prefix) and to safely handle overlapping public ranges by decomposing them into shared/non-shared regions backed by shared allocators. This aims to prevent public (IP,port) collisions across different VPCs that masquerade toward the same peer.

Changes:

Extend allocator lookups and reservations to include src_vpcd (fixes ambiguity when different VPCs reuse private space).
Build NAT pools in two passes: gather exposes per destination VPC, decompose overlapping public ranges into disjoint regions, and share region allocators across owners.
Add/expand tests covering shared public ranges, overlapping private prefixes, and partial public-range overlap behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
nat/src/masquerade/nf.rs	Passes `src_vpcd` into allocator allocation path for new sessions.
nat/src/masquerade/natip.rs	Adds `NatIp::to_addr_bits()` to avoid method-resolution pitfalls and support region math.
nat/src/masquerade/flows.rs	Includes `src_vpcd` when re-reserving allocations during allocator upgrades.
nat/src/masquerade/apalloc/test_alloc.rs	Updates fixtures/helpers for new pool-keying and adds overlap/collision regression tests.
nat/src/masquerade/apalloc/setup.rs	Reworks pool construction: gather exposes, decompose public ranges into regions, build per-region allocators, assign per-expose `PoolSet`s.
nat/src/masquerade/apalloc/region.rs	Introduces region decomposition logic and ordering preferences (exclusive regions first).
nat/src/masquerade/apalloc/mod.rs	Updates pool-table key to include `src_vpcd`, switches stored value to `PoolSet`, and updates allocation/reservation signatures.
nat/src/masquerade/apalloc/display.rs	Adds `Display` for `PoolSet` and updates key formatting to include source VPC.
nat/src/masquerade/apalloc/alloc.rs	Adds `PoolSet`/`PoolRegion` and changes `NatPool` construction to be per-region via `for_range`.

+            bitmap: PoolBitmap::with_offset_range(
+                to_offset(range.start),
+                to_offset(range.start + span),
+            ),


+    // Only the forward flow of a pair holds an allocation, and this is only reached for flows that
+    // have one, so the flow key's source really is the VPC the masqueraded traffic originates in.
+    let src_vpcd = flow_key.src_vpcd().unwrap_or_else(|| unreachable!());
    let dst_vpcd = flow_info.get_dst_vpcd().unwrap_or_else(|| unreachable!());


+        let src_vpcd = packet.meta().src_vpcd.unwrap_or_else(|| unreachable!());
        let dst_vpcd = packet.meta().dst_vpcd.unwrap_or_else(|| unreachable!());


One function, `set_bitmap_value`, took the value to write as an integer and got both directions wrong. - **Freeing never freed.** Clearing a bit was an OR with zero, which does nothing, so a port stayed marked used for the life of its block. Whole blocks are rebuilt once every port in them is returned, which is why a test that drops everything never saw it; a flow ending while its neighbours carry on is the ordinary case, and there the port was gone for good. - **Reserving a port already taken was allowed.** The guard compared the extracted bit, which is one shifted into position, against the value being written. Those agree only for the first port of each half of the bitmap, so for every other offset an already-set bit read as free. That error is what refuses a flow whose address and port have been taken, so instead of being refused it would be handed a pair another flow already holds. Takes a `bool` rather than an integer, which also removes the third value that had to be rejected at runtime. This sits at the bottom of the stack, ahead of everything that depends on it. The concurrent model checker cannot assert that a pair already held is refused a second time until this is in, and a suite that accepts such a success certifies the defect rather than catching it. The pool-level test for the freeing half needs machinery introduced further up and stays there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`#[concurrency::test]` wraps a test body in stress(), which is right when the body is the thing being model-checked. It is the wrong shape when a generator has to be the outer loop: a bolero suite calls stress() once per generated shape, from inside for_each. Wrapping that in stress() would put the whole campaign inside a single model-checking execution, making bolero's own choices part of the explored state space. Such suites therefore call stress() by hand from a plain #[test], and paid for it by losing the backend-named leaf the attribute appends. That leaf is load-bearing: 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. A suite without the leaf compiles under the feature and is never selected to run, which is what had happened to the flow table's concurrent fuzz suite: it has been built and skipped rather than run. Add model_test, which emits the same module shape and leaves the body verbatim. Unlike test, its wrapper is emitted on every backend, so the name does not change shape between them and the default backend gets a `plain` leaf; there is no existing flat name to stay compatible with. Apply it to the flow table suite, which now runs. Under shuttle it passes 1500 generated shapes, so this turns on a suite that works rather than one that needs fixing first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

An address and port pool was built per masquerade expose, and the exposes that shared a public range each got their own. Nothing tied those allocators together, so each of them believed it owned the whole range and allocated from it independently, handing out the same public address and port more than once. Two such flows towards the same remote endpoint then build the same reverse flow key. This is reachable. Exposes are only checked for collisions against the other exposes of the same manifest, and a manifest belongs to a single peering, so two VPCs that both peer with the same destination VPC can masquerade onto the same public range with nothing rejecting it. A VPC peering twice with the same peer cannot, since check_peering_count already refuses that. What an allocator hands out is a public (address, port) pair, so that is what decides whether two exposes describe the same pool. Register the allocators by (protocol, destination discriminant, public range) while the allocator is built, and hand the same one to every expose that claims that range. Pools are still looked up by private prefix, which is unavoidable: the private source address is all we have on the first packet of a flow. Ranges that overlap without being identical still get one allocator each, and are only reported, as sharing them needs the pools themselves to be restructured. Two exposes that share a public range but disagree on idle timeout or reserved ports now share a pool, and the policy declared first wins; a mismatched policy is worse than a duplicated allocation only in theory, so this is reported too. PoolTable::add_entry silently replaced an existing entry, which hid the private-side counterpart of this problem. Warn instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The pool serving a private source address was keyed by protocol, destination discriminant and private prefix, with nothing identifying the VPC the private address belongs to. A private address only means something within its own VPC, and two VPCs using the same private space is ordinary: it is much of what NAT is for. Two such VPCs peering with the same destination VPC therefore produced the same key, and the second silently replaced the first in the pool table. Traffic from the first VPC was then masqueraded onto the public range the second VPC exposes, which is both the wrong address and an allocation the first VPC's pool knows nothing about. Nothing rejects this configuration: exposes are only checked for collisions within a manifest, and each VPC is validated on its own. Carry the source discriminant in the pool table key, and order it before the address so the range lookup keeps scanning within a single pair of VPCs. The packet path and the flow re-reservation path both already had the source discriminant to hand. Only the forward flow of a pair holds an allocation, and re-reservation is only reached for flows that have one, so the flow key's source there is the originating VPC. Pool identity is deliberately left alone: it stays keyed on the public range, without the source discriminant, so that two VPCs masquerading onto one public range still share a single allocator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The pool-level guarantee behind the Clone removal in #1696. That change makes the sequence which broke it unwriteable, so what is left to check is the guarantee itself: a pair a live allocation holds cannot be reserved, and can be once it is released. Lives here rather than with the fix because it needs the claims argument `pool_sets_for_specs` grows in #1698.

The pool-level guarantee behind the Clone removal in #1696. That change makes the sequence which broke it unwriteable, so what is left to check is the guarantee itself: a pair a live allocation holds cannot be reserved, and can be once it is released. Lives here rather than with the fix because it needs the claims argument `pool_sets_for_specs` grows in #1698. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Exposes that masquerade towards the same peer VPC may claim public ranges that overlap only partially: 10.0.0.0/24 and 10.0.0.128/25 share half their addresses, and neither contains the other. Each expose got an allocator over its own range, so both believed they owned the shared half and could hand out the same public address and port. That misdelivers traffic between tenants. Return traffic is matched on a reverse flow key built from the peer VPC, the remote endpoint and the public address and port; nothing in it identifies the VPC the traffic came from, and the peer cannot supply it. Two flows that collide on that key are indistinguishable, and the flow table replaces the older entry, so one tenant's return traffic is translated to the other tenant's private address and delivered into its VPC. Cut the public space instead. For each peer VPC, every point where the set of exposes covering an address changes becomes a boundary, which yields maximal intervals with a constant set of owners and no overlap between them. One allocator per region then keeps allocations unique, and an expose allocates from the regions its own range covers, so it is still only ever given an address it is configured for. Regions an expose does not share are offered first, which keeps VPCs off each other's locks and leaves shared space for exposes that have nowhere else to go. Pools can no longer be built one expose at a time, since regions depend on every expose claiming that space, so building happens in two passes. Identical ranges are now just the degenerate case of overlap, which retires the pool registry keyed on the public range. The idle timeout moves from the pool to the expose. It is a per-expose setting and regions are shared, so it could not stay on something two exposes have in common. Regions are contiguous, which lets the IPv6 bitmap index carry a single entry per region rather than one per prefix. The decomposition is over addresses only. Public ranges can also carry port ranges, which the pools still do not model. The tests come with it. Two VPCs peering with the same destination VPC, one masquerading onto 10.1.0.0/30 and the other onto 10.1.0.2/31 so that neither range contains the other, check end to end that the space is cut as expected, that the shared region is backed by one allocator rather than a copy per VPC, that a VPC draws from space it does not share before falling back to shared space, and that neither is given an address outside the range its own expose declares. Property tests cover the same ground without the cases being chosen by hand. They draw ranges from a narrow window, so that overlap is the common case rather than astronomically unlikely and every property can be checked against every address in the window; the window sits at both ends of the address space, which exercises the cut just past the end of a range where it would overflow. The properties are the ones return traffic depends on: regions never overlap, an address is owned by exactly the exposes that claimed it, an expose is offered exactly the regions it owns and never an address outside the ranges it declares, and no public address and port is handed out twice. A config update is covered too, since that is where an allocation has to survive being carried from one allocator to the next. So that the tests drive the real construction rather than a copy of it, the part of the build that turns public ranges into pools is split out as pool_sets_for_specs(), taking a description of an expose rather than config types. Two further tests are committed ignored, because they fail for a reason that has nothing to do with regions: a pool holds one reserved port range per public address, so of several port-forwarding claims on one address only the last is honoured. They stay latent in production because the claims are computed from private prefixes and never match the public address they are looked up by. Both are fixed later in this series, and the notes on them say what has to change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

The pool-level guarantee behind the Clone removal in #1696. That change makes the sequence which broke it unwriteable, so what is left to check is the guarantee itself: a pair a live allocation holds cannot be reserved, and can be once it is released. Lives here rather than with the fix because it needs the claims argument `pool_sets_for_specs` grows in #1698. 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>

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. which is the wrong shape when bolero has to be the outer loop; model_test emits the same leaf and leaves the body verbatim. The uniqueness oracle needed one more thing before it was worth trusting. 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 Opus 5 (1M context) <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. A fourth site, found by review once the three above were fixed. 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>

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>

The pool-level guarantee behind the Clone removal in #1696. That change makes the sequence which broke it unwriteable, so what is left to check is the guarantee itself: a pair a live allocation holds cannot be reserved, and can be once it is released. Lives here rather than with the fix because it needs the claims argument `pool_sets_for_specs` grows in #1698. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland self-assigned this Aug 5, 2026

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

daniel-noland added the ci:+release Enable VLAB release tests label Aug 5, 2026

daniel-noland requested a review from Copilot August 5, 2026 21:15

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot started reviewing on behalf of daniel-noland August 5, 2026 21:20 View session

daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch 3 times, most recently from ff8bdcb to 33a926f Compare August 6, 2026 02:27

daniel-noland changed the title ~~port allocator fixes~~ masquerade: allocator collision fixes Aug 6, 2026

daniel-noland changed the title ~~masquerade: allocator collision fixes~~ (1)-masquerade: allocator collision fixes Aug 6, 2026

daniel-noland and others added 4 commits August 6, 2026 15:12

daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 33a926f to 191e004 Compare August 6, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 5173d8e to 1ca53df Compare August 6, 2026 23:51

daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 1ca53df to 44c59e4 Compare August 7, 2026 01:07

daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 44c59e4 to 090a2f7 Compare August 7, 2026 01:30

daniel-noland and others added 7 commits August 6, 2026 19:42

daniel-noland force-pushed the pr/daniel-noland/port-allocator-fixes branch from 090a2f7 to d42aea5 Compare August 7, 2026 01:50

daniel-noland changed the title ~~(1)-masquerade: allocator collision fixes~~ (1)-masquerade: allocate each public pair once Aug 7, 2026

daniel-noland commented Aug 7, 2026

View reviewed changes

Comment thread nat/src/masquerade/apalloc/setup.rs Outdated

Navigation Menu

Uh oh!

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

(1)-masquerade: allocate each public pair once#1696
daniel-noland wants to merge 12 commits into
mainfrom
pr/daniel-noland/port-allocator-fixes

daniel-noland commented Aug 5, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let src_vpcd = packet.meta().src_vpcd.unwrap_or_else(\|\| unreachable!());
		let dst_vpcd = packet.meta().dst_vpcd.unwrap_or_else(\|\| unreachable!());

Uh oh!

Conversation

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

Uh oh!

The allocator can deadlock against its own pool lock

Stack

Uh oh!

coderabbitai Bot commented Aug 5, 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

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniel-noland commented Aug 5, 2026 •

edited

Loading

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading