fix(masquerade): Close the window where an address belongs to neither list - #1743
fix(masquerade): Close the window where an address belongs to neither list#1743daniel-noland wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Loom 0.7 ships no `Weak`, so this crate supplies one, and it was missing the count. A caller that needs to ask "has the last strong reference gone?" while holding a lock that the value's `Drop` also takes cannot answer it with `upgrade`: the temporary `Arc` that mints may be the last one, and dropping it runs `Drop` re-entrantly into that same lock. Reading the count answers the question without taking a reference at all. Under this shim the answer is always "no", because the `Weak` holds a strong clone -- the same divergence the module header already records for `upgrade` and `Arc::strong_count`. That makes such a path inert under loom rather than wrong, and the note says so at both the method and the module level so nobody concludes from a passing loom run that the path was exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
… list
A `NatPool` decides whether a public address can be handed out by consulting
two structures: the bitmap of free addresses, and the list of weak references
to the addresses in use. An address is meant to be in exactly one of them.
Releasing one passes through a state where it is in neither. `Weak::upgrade`
stops resolving the instant the last `AllocatedPort` takes the strong count
to zero, but the address only returns to the bitmap later, inside
`AllocatedIp::drop`, which must first acquire the pool's write lock -- a lock
every allocation also takes, on every call, via `cleanup_used_ips`. An
allocation arriving in between finds nothing on either path and reports
exhaustion while the pool is holding an address that belongs to no one.
That window is not a rare interleaving. `Arc` fixes the ordering, so every
release passes through it; only its duration varies, bounded by how long the
releasing thread waits for a contended lock. Where an expose gives a peering
a single public address, the unavailable address is the whole pool: the
packet is dropped and the flow refused.
The underlying mistake is inferring availability from `Arc` liveness, which
changes on whatever thread drops the last reference, when availability is a
property of the pool, which changes only under its lock. The two could
therefore disagree.
Give each lease of an address a `Tenancy`, recorded with the address's
bitmap offset beside the weak reference, and keep the current tenancies in a
map updated in the same critical section as the bitmap. The pool's own state
becomes the source of truth, and the hand-back becomes completable from
either side: an allocation holding the lock that finds a dead weak reference
finishes the hand-back itself rather than waiting for the releasing thread to
win the lock, and `use_new_ip` reclaims ended tenancies before reporting an
empty bitmap -- in the same critical section as the retry, so the answer
cannot go stale in between. A late `AllocatedIp::drop` whose tenancy has been
retired does nothing, which is what makes the takeover safe.
That last point also settles a hazard `reserve_from_pool` had documented as
harmless: it creates a second `AllocatedIp` for an address whose owner it
cannot find, and the first one's `Drop` would then free an address the second
still held.
Two smaller corrections ride along, both about being able to see what
happened:
- The reuse scan ended at the first address that had run out of port
blocks, stranding every address behind it in the list. Running out of
blocks is exhaustion like any other; keep scanning.
- Drawing a fresh address reports `NoFreeIp` whenever the bitmap is empty,
which for a single-address pool is always, and that answer buried the
reason the addresses already in use could not serve the request. Report
whichever of the two is the more specific.
The regression test is a one-address pool with one thread releasing the only
allocation while another allocates, which is the shape DNS traffic takes: a
reply from port 53 moves the flow to `Closed` and invalidates the pair at
once to conserve ports, so such a pool destroys and rebuilds its address
about once per query. Against the unfixed allocator it fails under shuttle
with the error seen in the field, `NoFreeIp`.
The behaviour was also confirmed on a live gateway by deliberately widening
the window: the unpatched allocator refused ten of twenty queries with the
drop counter and the log agreeing exactly, while the patched allocator served
all twenty under the identical conditions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
45c8b4c to
00963f4
Compare
|
Thanks — both blockers confirmed and fixed, plus the three non-blocking notes. Force-pushed as
I took the "extend the shim" option, for the reason given in the review:
DCO. Both commits now carry Non-blocking, all three taken:
Verified on One correction to the record: the review noting "no workspace files were changed" is no longer true — the loom shim touches |
|
Superseded by #1744, opened from an |
1 participant
Issue
NatPoolanswers "is this public address available?" from two structures — a bitmap of free addresses and a list of weak references to the addresses currently in use — and an address is meant to be in exactly one of them. Releasing one passes through a state where it is in neither:Weak::upgradestops resolving the instant the lastAllocatedPortdrops the strong count to zero, but the address only returns to the bitmap later, insideAllocatedIp::drop, which must first acquire the pool's write lock. An allocation arriving in that gap finds nothing on either path and is told the pool is empty, so it fails withNoFreeIpwhile the pool is in fact holding an address that belongs to no one. This is not a rare interleaving —Arcguarantees the ordering, so every release passes through the gap; only its duration varies, bounded by how long the releasing thread waits for a write lock that every allocation also takes, on every call, viacleanup_used_ips. The same reasoning covers a second latent hazard the fix closes:reserve_from_poolcould create a duplicateAllocatedIpfor an address whose owner had not yet finished releasing it, after which the first object'sDropwould free an address the second still held.Impact
The packet that lands in the gap is dropped with
DoneReason::NatOutOfResourcesand its flow is refused, logged asmasquerade: Ip/port allocation failed ... no free IP available. Two properties of real deployments turn a narrow race into a recurring one. Where an expose is configured with a single public address per peering — the shape observed in the field — the unavailable address is the entire pool, so there is nothing to fall back on. And DNS drives the window open constantly: a reply from source port 53 moves the flow toClosedand invalidates the pair immediately as a port-conservation measure, so a pool carrying mostly DNS destroys and rebuilds its address roughly once per query, opening one window per query. The user-visible effect is mild and easily misread — resolvers retry, so lookups still succeed and the failures surface only as intermittent warnings and a raised drop count, at a rate that does not correlate with load in any obvious way.Fix
The root problem is that liveness was inferred from
Arcrefcounts, which change on whatever thread happens to drop the last reference, while availability is a property of the pool, which changes only under its lock — so the two could disagree. The fix gives every lease of an address aTenancy, recorded with the address's bitmap offset alongside the weak reference, and keeps a map of current tenancies that is updated in the same critical section as the bitmap itself. That makes the pool's own state the source of truth and lets the hand-back be completed from either side: an allocation that holds the lock and finds a dead weak reference finishes the hand-back itself rather than waiting for the releasing thread to win the lock, anduse_new_ipreclaims ended tenancies before it reports an empty bitmap — in the same critical section as the retry, so the answer cannot go stale between the two. A lateAllocatedIp::dropwhose tenancy has since been retired does nothing, which is what makes that takeover safe and, incidentally, what closes the duplicate-AllocatedIphazard inreserve_from_pool.Two smaller corrections ride along, both about being able to see what happened: the reuse scan no longer abandons the search at the first address that has run out of port blocks, stranding every address behind it in the list, and
NoFreeIpno longer buries the more specific reason — which, on a single-address pool, it always did.Testing
an_allocation_racing_the_last_release_is_still_served: a one-address pool with one thread releasing the only allocation while another allocates. Against the unfixed allocator it reproduces the field error verbatim (NoFreeIp); against this change it passes.cargo test -p dataplane-nat --lib— 165 pass.cargo test -p dataplane-nat --features shuttle --lib shuttle— 8 pass.cargo clippy -p dataplane-nat --all-targets— clean.Notes
v0.25.2is prepared onbackport/v0.25.2/masquerade-address-handback; it is semantically identical (the two differ only by one clarifying comment).🤖 Generated with Claude Code