From 096d24f0148b69d0b0b2e85468469bd9ec058fa1 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 16:01:11 -0600 Subject: [PATCH 01/24] test(masquerade): Model-check the pools across a config change 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 419 ++++++++++++++++++ nat/src/masquerade/apalloc/mod.rs | 1 + nat/src/masquerade/apalloc/setup.rs | 1 + nat/src/masquerade/apalloc/test_alloc.rs | 9 + 4 files changed, 430 insertions(+) create mode 100644 nat/src/masquerade/apalloc/concurrent_fuzz.rs diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs new file mode 100644 index 0000000000..1e4b194db7 --- /dev/null +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -0,0 +1,419 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Concurrent fuzz test for the masquerade pools across a config change. +//! +//! One test, [`stress_test_config_change`], drives a bolero-generated [`Scenario`] through +//! [`concurrency::stress`] on every backend, mirroring the bolero x model-checker layout used for +//! the flow table (see `flow-entry/src/flow_table/concurrent_fuzz.rs`). bolero is the *outer* loop +//! and picks the shape: which public ranges the exposes claim, and an op stream per thread. The +//! backend is the *inner* loop and explores interleavings of that fixed shape: +//! +//! * **default (std) backend** — one direct run on real OS threads. Build with +//! `just test sanitize=thread` to surface data races inside the allocator. +//! * **`--features shuttle`** — the full portfolio (Random + PCT [+ DFS]). +//! +//! # What is being raced +//! +//! 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 over into it by re-reserving the address +//! and port each one holds, and only then publishes it; meanwhile packet threads keep allocating +//! from whichever allocator is currently published. Every lock and atomic the allocator uses comes +//! from `concurrency::sync`, so a model checker sees all of it: 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 that was carried over into it. This +//! is the safety property of the update: the writer re-reserves before publishing, so a flow that +//! survived a config change and a flow created just after it must not collide on the reverse key. +//! * An address and port is never handed to two live flows drawn from the same allocator. +//! * Neither allocation nor reservation ever reports [`AllocatorError::InternalIssue`]. That is the +//! allocator saying its own bookkeeping is inconsistent, and `find_block_for_port` carries a +//! standing `FIXME` wondering whether the block it just found non-free can be released before it +//! is looked up. Reserving concurrently with allocating is what would show it. +//! +//! # No loom +//! +//! Gated off under loom, for the same reason `test_alloc`'s concurrency tests are: loom's `Weak` +//! shim never lets an allocator liveness entry die, so the pool's in-use list never drains and the +//! run does not model what production does. Shuttle has no such limitation. +//! +//! # Why `#[concurrency::model_test]` +//! +//! `just features=shuttle test` filters the run down to test names containing `shuttle`, because +//! under that backend `concurrency::sync` types are shuttle primitives and every other test in the +//! workspace would fail spuriously on `ExecutionState NotSet`. `#[concurrency::test]` earns its +//! way past that filter by appending a `concurrency_model::shuttle` leaf, but it also wraps the +//! whole body in [`concurrency::stress`], which is the wrong shape here: bolero has to be the outer +//! loop, so `stress` is called once per generated shape from inside it. +//! [`macro@concurrency::model_test`] emits the same backend-named leaf and leaves the body alone, +//! which is what lets this suite be selected at all. + +#![cfg(test)] +#![cfg(not(feature = "loom"))] + +use super::AllocatedPort; +use super::alloc::PoolSet; +use super::region::AddrInterval; +use super::setup::{PoolSpec, pool_sets_for_specs}; +use crate::masquerade::allocation::AllocatorError; +use crate::port::NatPort; +use concurrency::slot::SlotOption; +use concurrency::sync::{Arc, Mutex}; +use concurrency::thread; +// `spawn_scoped` is inherent on std's `Builder`, but supplied by `BuilderExt` under shuttle +#[cfg_attr(not(feature = "shuttle"), allow(unused_imports))] +use concurrency::thread::BuilderExt; +use lpm::prefix::PrefixPortsSet; +use net::ip::NextHeader; +use std::collections::BTreeSet; +use std::net::Ipv4Addr; +use std::time::Duration; + +// 10.1.0.0, over a window narrow enough that generated ranges overlap most of the time and the +// pools stay cheap to build once per published generation. +const BASE: u128 = 0x0A01_0000; +const WINDOW: u128 = 8; +const MAX_EXPOSES: u8 = 3; +const MAX_RANGE_LEN: u8 = 4; + +// Op streams are kept short: the backend explores interleavings of a fixed shape, so length costs +// schedule space without buying coverage. +const MAX_PACKET_OPS: usize = 6; +const MAX_CONFIG_OPS: usize = 3; +const PACKET_WORKERS: usize = 2; + +const IDLE_TIMEOUT: Duration = Duration::from_mins(2); + +/// What a packet thread does. Allocating and freeing model flows starting and ending; reserving +/// models a flow being carried over, and is the op that races reservation against allocation on +/// pools that are already published and in use. +#[derive(Clone, Copy, Debug, bolero::TypeGenerator)] +enum PacketOp { + Allocate, + FreeOldest, + ReserveExisting, +} + +/// What the config thread does. Production has a single writer, so only this thread republishes. +#[derive(Clone, Copy, Debug, bolero::TypeGenerator)] +enum ConfigOp { + Republish, + Idle, +} + +/// One generated shape: the public ranges each expose claims, and an op stream per thread. +#[derive(Clone, Debug)] +struct Scenario { + ranges: Vec>, + packet_ops: [Vec; PACKET_WORKERS], + config_ops: Vec, +} + +impl bolero::TypeGenerator for Scenario { + /// Generate a shape, then normalize it so the run always exercises real concurrency. + /// + /// shuttle's PCT scheduler panics on a body in which two threads are never simultaneously + /// runnable. Rather than skip degenerate shapes, every packet stream is given an `Allocate` if + /// it has none, and the config stream a `Republish`. The splice position comes from the driver, + /// so the normalization stays a deterministic function of the input and a failure still + /// reproduces from its seed. + fn generate(driver: &mut D) -> Option { + let expose_count = usize::from(driver.produce::()? % MAX_EXPOSES + 1); + let mut ranges = Vec::with_capacity(expose_count); + for _ in 0..expose_count { + let offset = driver.produce::()? % u8::try_from(WINDOW).ok()?; + let length = driver.produce::()? % MAX_RANGE_LEN + 1; + ranges.push(vec![(offset, length)]); + } + + let mut packet_ops: [Vec; PACKET_WORKERS] = driver.produce()?; + for ops in &mut packet_ops { + ops.truncate(MAX_PACKET_OPS); + if !ops.iter().any(|op| matches!(op, PacketOp::Allocate)) { + let at = driver.produce::()? % (ops.len() + 1); + ops.insert(at, PacketOp::Allocate); + } + } + + let mut config_ops: Vec = driver.produce()?; + config_ops.truncate(MAX_CONFIG_OPS); + if !config_ops + .iter() + .any(|op| matches!(op, ConfigOp::Republish)) + { + let at = driver.produce::()? % (config_ops.len() + 1); + config_ops.insert(at, ConfigOp::Republish); + } + + Some(Self { + ranges, + packet_ops, + config_ops, + }) + } +} + +/// One generation of published pools, together with the flows carried into it. +/// +/// The reservations are held for as long as the generation is published, exactly as a surviving +/// flow holds the allocation it was re-reserved. +struct Published { + generation: u64, + pools: Vec>, + carried: BTreeSet<(Ipv4Addr, u16)>, + _reservations: Vec>, +} + +impl Published { + /// Build the pools for a new config and carry the surviving flows into them before returning, + /// so that a generation is only ever published once its survivors hold their addresses again. + fn build( + specs: &[PoolSpec], + generation: u64, + survivors: &[(usize, Ipv4Addr, NatPort)], + ) -> Self { + let pools = pool_sets_for_specs::(specs, NextHeader::TCP, false); + let mut carried = BTreeSet::new(); + let mut reservations = Vec::new(); + + for &(owner, ip, port) in survivors { + let Some(pool) = pools.get(owner) else { + continue; + }; + match pool.reserve(ip, port) { + Ok(reservation) => { + carried.insert((ip, port.as_u16())); + reservations.push(reservation); + } + Err(AllocatorError::InternalIssue(message)) => { + panic!("re-reserving {ip}:{port} for generation {generation}: {message}") + } + // The address may no longer be served, or another survivor may already hold it. + Err(_) => {} + } + } + + Self { + generation, + pools, + carried, + _reservations: reservations, + } + } +} + +/// Every address and port currently held by a flow, with the generation it was drawn from. +/// +/// Shared by all threads, because a collision between two threads is the interesting one and a +/// per-thread record would not see it. Two allocations from *different* generations may legitimately +/// repeat: only the survivors carried into a new generation are protected there, which is what +/// [`Published::carried`] covers. +struct Live(Mutex>); + +impl Live { + fn new() -> Self { + Self(Mutex::new(BTreeSet::new())) + } + + /// Record a freshly allocated pair. The allocation already exists by the time this runs, so a + /// thread that raced us to the same pair has either recorded it already or is about to fail + /// here itself; either way the collision is caught. + fn claim(&self, generation: u64, ip: Ipv4Addr, port: u16) { + let mut live = self.0.lock(); + assert!( + live.insert((generation, ip, port)), + "generation {generation} handed out {ip}:{port} to two flows at once" + ); + } + + /// Give a pair back, freeing it while the record is still locked so that no other thread can + /// claim it before the allocator has actually released it. + fn release(&self, generation: u64, allocation: AllocatedPort) { + let mut live = self.0.lock(); + live.remove(&(generation, allocation.ip(), allocation.port().as_u16())); + drop(allocation); + } +} + +impl Scenario { + fn specs(&self) -> Vec { + self.ranges + .iter() + .map(|ranges| PoolSpec { + public_ranges: ranges + .iter() + .map(|&(offset, length)| { + let start = u128::from(offset); + let end = (start + u128::from(length) - 1).min(WINDOW - 1); + AddrInterval::new(BASE + start, BASE + end) + }) + .collect(), + reserved: PrefixPortsSet::new(), + idle_timeout: IDLE_TIMEOUT, + }) + .collect() + } + + /// Run the scenario: stand up a first generation with a few flows already on it, then let the + /// packet threads and the config thread work against the published slot concurrently. + fn run(&self) { + let specs = self.specs(); + + // The flows that already exist when the config change arrives. + let initial = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let mut existing = Vec::new(); + let mut survivors = Vec::new(); + for (owner, pool) in initial.iter().enumerate() { + if let Ok(allocation) = pool.allocate(false) { + survivors.push((owner, allocation.ip(), allocation.port())); + existing.push(allocation); + } + } + + let slot = Arc::new(SlotOption::new(Some(Arc::new(Published::build( + &specs, 0, &survivors, + ))))); + let live = Arc::new(Live::new()); + + thread::scope(|scope| { + let mut handles = Vec::new(); + + for (index, ops) in self.packet_ops.iter().enumerate() { + let slot = slot.clone(); + let live = live.clone(); + let ops = ops.clone(); + let survivors = survivors.clone(); + handles.push( + thread::Builder::new() + .name(format!("packet-{index}")) + .spawn_scoped(scope, move || { + packet_worker(&slot, &live, &ops, &survivors); + }) + .expect("spawn packet worker"), + ); + } + + { + let slot = slot.clone(); + let ops = self.config_ops.clone(); + let specs = specs.clone(); + let survivors = survivors.clone(); + handles.push( + thread::Builder::new() + .name("config".to_string()) + .spawn_scoped(scope, move || { + let mut generation = 0u64; + for op in ops { + match op { + ConfigOp::Republish => { + generation += 1; + let next = Published::build(&specs, generation, &survivors); + slot.store(Some(Arc::new(next))); + } + ConfigOp::Idle => {} + } + thread::yield_now(); + } + }) + .expect("spawn config worker"), + ); + } + + for handle in handles { + handle.join().expect("worker panicked"); + } + }); + + drop(existing); + } +} + +fn packet_worker( + slot: &SlotOption, + live: &Live, + ops: &[PacketOp], + survivors: &[(usize, Ipv4Addr, NatPort)], +) { + // What this thread is holding, tagged with the generation it was drawn from. + let mut held: Vec<(u64, AllocatedPort)> = Vec::new(); + + for (step, op) in ops.iter().enumerate() { + let published = slot.load_full().expect("pools are always published"); + + match op { + PacketOp::Allocate => { + let owner = step % published.pools.len(); + match published.pools[owner].allocate(false) { + Ok(allocation) => { + let pair = (allocation.ip(), allocation.port().as_u16()); + + // The writer re-reserved every survivor before publishing, so a new flow + // must never be handed what a carried-over flow still holds. + assert!( + !published.carried.contains(&pair), + "generation {} handed out {}:{}, which a carried-over flow holds", + published.generation, + pair.0, + pair.1 + ); + + live.claim(published.generation, pair.0, pair.1); + held.push((published.generation, allocation)); + } + Err(AllocatorError::InternalIssue(message)) => { + panic!( + "allocating from generation {}: {message}", + published.generation + ) + } + // Running out of addresses or ports is a legitimate outcome. + Err(_) => {} + } + } + PacketOp::FreeOldest => { + if !held.is_empty() { + let (generation, allocation) = held.remove(0); + live.release(generation, allocation); + } + } + PacketOp::ReserveExisting => { + // Race a reservation against the other threads' allocations on pools that are + // already published and in use. Failing is fine, claiming inconsistent bookkeeping + // is not. + if let Some(&(owner, ip, port)) = survivors.get(step % survivors.len().max(1)) + && let Some(pool) = published.pools.get(owner) + && let Err(AllocatorError::InternalIssue(message)) = pool.reserve(ip, port) + { + panic!( + "reserving {ip}:{port} in generation {}: {message}", + published.generation + ); + } + } + } + + // Give the model checker a preemption point between ops; a cheap hint under std. + thread::yield_now(); + } + + // Give everything back through the record, so a thread that finishes early cannot leave + // entries behind and make a later, legitimate allocation look like a collision. + for (generation, allocation) in held { + live.release(generation, allocation); + } +} + +#[concurrency::model_test] +fn stress_test_config_change() { + bolero::check!() + .with_type() + .cloned() + .for_each(|scenario: Scenario| { + concurrency::stress(move || { + scenario.run(); + }); + }); +} diff --git a/nat/src/masquerade/apalloc/mod.rs b/nat/src/masquerade/apalloc/mod.rs index 82ff031a36..d778a182c2 100644 --- a/nat/src/masquerade/apalloc/mod.rs +++ b/nat/src/masquerade/apalloc/mod.rs @@ -97,6 +97,7 @@ use tracectl::trace_target; trace_target!("nat-allocation", LevelFilter::ERROR, &["masquerade"]); mod alloc; +mod concurrent_fuzz; mod display; mod natip_with_bitmap; mod pool_fuzz; diff --git a/nat/src/masquerade/apalloc/setup.rs b/nat/src/masquerade/apalloc/setup.rs index 901c1bda65..f5b3a32ca0 100644 --- a/nat/src/masquerade/apalloc/setup.rs +++ b/nat/src/masquerade/apalloc/setup.rs @@ -200,6 +200,7 @@ fn build_pools_generic<'a, I, J, F, FIter, P, PIter>( /// What building a pool needs to know about one expose, independent of where it came from. Keeping /// this free of config types is what lets the property tests drive the real construction. +#[derive(Clone)] pub(crate) struct PoolSpec { pub(crate) public_ranges: Vec, pub(crate) reserved: PrefixPortsSet, diff --git a/nat/src/masquerade/apalloc/test_alloc.rs b/nat/src/masquerade/apalloc/test_alloc.rs index 3524cf4088..41902accf2 100644 --- a/nat/src/masquerade/apalloc/test_alloc.rs +++ b/nat/src/masquerade/apalloc/test_alloc.rs @@ -40,6 +40,7 @@ mod context { pub fn vni2() -> Vni { Vni::new_checked(200).unwrap() } + #[allow(dead_code)] pub fn vni3() -> Vni { Vni::new_checked(300).unwrap() } @@ -49,6 +50,7 @@ mod context { pub fn vpcd2() -> VpcDiscriminant { VpcDiscriminant::from_vni(vni2()) } + #[allow(dead_code)] pub fn vpcd3() -> VpcDiscriminant { VpcDiscriminant::from_vni(vni3()) } @@ -160,6 +162,7 @@ mod context { // exposes across VPCs: collisions are only checked between the exposes of a single manifest. // Both peerings therefore reach the allocator with the same destination discriminant and the // same public range, which is one pool described twice. + #[allow(dead_code)] fn build_context_shared_public_range() -> ValidatedVpcTable { let masquerade_manifest = |name: &str, private: &str| { VpcManifest::with_exposes( @@ -208,6 +211,7 @@ mod context { vpctable.validate().unwrap() } + #[allow(dead_code)] pub fn build_allocator_shared_public_range() -> NatAllocator { let vpc_table = build_context_shared_public_range(); // Without randomization the first port block picked is deterministic, so two pools that @@ -219,6 +223,7 @@ mod context { // Two VPCs using the *same* private prefix, each peering with the same destination VPC and // masquerading onto a public range of its own. Tenants reusing private address space is // ordinary, and is much of what NAT is for, so both are entitled to their own pool. + #[allow(dead_code)] fn build_context_overlapping_private_prefixes() -> ValidatedVpcTable { let masquerade_manifest = |name: &str, public: &str| { VpcManifest::with_exposes( @@ -267,6 +272,7 @@ mod context { vpctable.validate().unwrap() } + #[allow(dead_code)] pub fn build_allocator_overlapping_private_prefixes() -> NatAllocator { let vpc_table = build_context_overlapping_private_prefixes(); let config = MasqueradeConfig::new(&vpc_table, 1).set_randomize(false); @@ -276,6 +282,7 @@ mod context { // Two VPCs peering with the same destination VPC, masquerading onto public ranges that overlap // *partially*: VPC-1 takes 10.1.0.0/30 (.0 through .3) and VPC-2 takes 10.1.0.2/31 (.2 and // .3). Neither range contains the other, so no single range identifies the shared space. + #[allow(dead_code)] fn build_context_partial_overlap() -> ValidatedVpcTable { let masquerade_manifest = |name: &str, private: &str, public: &str| { VpcManifest::with_exposes( @@ -324,12 +331,14 @@ mod context { vpctable.validate().unwrap() } + #[allow(dead_code)] pub fn build_allocator_partial_overlap() -> NatAllocator { let vpc_table = build_context_partial_overlap(); let config = MasqueradeConfig::new(&vpc_table, 1).set_randomize(false); NatAllocator::new(config) } + #[allow(dead_code)] pub fn get_pool_set_v4( pool: &PoolTable, src_vpcd: VpcDiscriminant, From 823929a1db414f49f5542feef7f57ff7ed948102 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 14:11:38 -0600 Subject: [PATCH 02/24] fix(masquerade): Stop the allocator deadlocking against its own pool 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 102 ++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 27 deletions(-) diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 01a46963fe..42cbe8f675 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -58,25 +58,46 @@ impl IpAllocator { &self, allow_null: bool, ) -> Result, AllocatorError> { - let allocated_ips = self.pool.read(); - for ip_weak in allocated_ips.ips_in_use() { - let Some(ip) = ip_weak.upgrade() else { - continue; - }; - if !ip.has_free_ports() { - continue; - } - match ip.allocate_port_for_ip(allow_null) { - Ok(port) => { - debug!("Allocated port {port}"); - return Ok(port); + // An address upgraded out of the in-use list has to outlive the guard below. + // + // The list holds weak references; the strong ones belong to the blocks handed out from + // each address. Another thread ending the last flow on an address drops the last of those + // at any moment, which leaves the reference upgraded here as the only one. Letting it go + // while the guard is held runs `AllocatedIp::drop` on this thread, and that takes the same + // lock for writing: a self-deadlock that wedges the core for good, on the path every new + // flow takes. + // + // Every upgrade is therefore kept until the guard is gone, and released after it. + let mut examined: Vec>> = Vec::new(); + let outcome = { + let allocated_ips = self.pool.read(); + let mut outcome = Err(AllocatorError::NoFreeIp); + for ip_weak in allocated_ips.ips_in_use() { + let Some(ip) = ip_weak.upgrade() else { + continue; + }; + examined.push(ip.clone()); + if !ip.has_free_ports() { + continue; + } + match ip.allocate_port_for_ip(allow_null) { + Ok(port) => { + debug!("Allocated port {port}"); + outcome = Ok(port); + break; + } + // If there is no free port left, loop again to try another IP address + Err(AllocatorError::NoFreePort(_)) => {} + Err(e) => { + outcome = Err(e); + break; + } } - // If there is no free port left, loop again to try another IP address - Err(AllocatorError::NoFreePort(_)) => {} - Err(e) => return Err(e), } - } - Err(AllocatorError::NoFreeIp) + outcome + }; + drop(examined); + outcome } fn allocate_new_ip_from_pool(&self) -> Result>, AllocatorError> { @@ -97,8 +118,16 @@ impl IpAllocator { } fn cleanup_used_ips(&self) { - let mut allocated_ips = self.pool.write(); - allocated_ips.cleanup(); + // Same trap as in `reuse_allocated_ip`, and worse: `cleanup` upgrades each weak reference + // to see whether it still resolves, and does it holding the pool's *write* lock. An + // upgrade that turns out to be the last strong reference runs `AllocatedIp::drop` on this + // thread, which asks for that same lock again. + let mut released = Vec::new(); + { + let mut allocated_ips = self.pool.write(); + allocated_ips.cleanup(&mut released); + } + drop(released); } pub(crate) fn allocate( @@ -115,9 +144,15 @@ impl IpAllocator { } fn get_allocated_ip(&self, ip: I) -> Result>, AllocatorError> { - self.pool - .write() - .reserve_from_pool(ip, self.clone(), self.randomize) + // The third place that upgrades an in-use entry under the pool lock, and so the third that + // must not let the upgrade go while holding it. See `cleanup_used_ips`. + let mut examined = Vec::new(); + let outcome = + self.pool + .write() + .reserve_from_pool(ip, self.clone(), self.randomize, &mut examined); + drop(examined); + outcome } pub(crate) fn reserve( @@ -365,8 +400,16 @@ impl NatPool { self.in_use.push_back(Arc::downgrade(ip)); } - fn cleanup(&mut self) { - self.in_use.retain(|ip| ip.upgrade().is_some()); + /// Drop the entries whose addresses are gone, handing the caller every address that is still + /// alive so it can release them once the pool lock is no longer held. See `cleanup_used_ips`. + fn cleanup(&mut self, keep_alive: &mut Vec>>) { + self.in_use.retain(|ip| match ip.upgrade() { + Some(alive) => { + keep_alive.push(alive); + true + } + None => false, + }); } pub(crate) fn ips_in_use(&self) -> impl Iterator>> { @@ -416,18 +459,23 @@ impl NatPool { self.bitmap.set_ip_free(offset); } + /// `keep_alive` collects every address upgraded here, for the caller to release once the pool + /// lock is gone. See `cleanup_used_ips`. fn reserve_from_pool( &mut self, ip: I, ip_allocator: IpAllocator, randomize: bool, + keep_alive: &mut Vec>>, ) -> Result>, AllocatorError> { let offset = I::try_to_offset(ip, &self.reverse_bitmap_mapping)?; for ip_weak in self.ips_in_use() { - if let Some(ip_arc) = ip_weak.upgrade() - && ip_arc.ip() == ip - { + let Some(ip_arc) = ip_weak.upgrade() else { + continue; + }; + keep_alive.push(ip_arc.clone()); + if ip_arc.ip() == ip { // We found the allocated IP in the list of IPs in use, return it debug!("Reserved ip {ip_arc}"); return Ok(ip_arc); From ee5dbf27bdd93d32b441e834d5536199972cbef2 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 17:35:34 -0600 Subject: [PATCH 03/24] fix(masquerade): Keep printing the pool from wedging it too 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 43 +++++++++++++++++++ nat/src/masquerade/apalloc/display.rs | 21 ++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs index 1e4b194db7..f5ebaa494c 100644 --- a/nat/src/masquerade/apalloc/concurrent_fuzz.rs +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -417,3 +417,46 @@ fn stress_test_config_change() { }); }); } + +/// Printing the allocator races the last flow on an address ending. +/// +/// The pool holds weak references to the addresses in use, and printing upgrades each one while +/// the pool's read guard is held. An address whose last block is released at that moment leaves +/// the upgrade taken for printing as the only strong reference, and dropping it runs +/// `AllocatedIp::drop` on the printing thread, which takes the same lock for writing. +/// +/// This is the same self-deadlock the allocation paths guard against, reached from the management +/// side: `NatAllocator` is a `CliSource`, so the table is formatted on a thread of its own while +/// packet threads keep ending flows. A wedged read guard takes the pool with it. +/// +/// Shuttle names it directly -- "tried to acquire a `RwLock` it already holds" -- so the +/// assertion is the run completing at all. +#[concurrency::model_test] +fn printing_the_pool_does_not_wedge_it_against_a_flow_ending() { + concurrency::stress(|| { + let specs = vec![PoolSpec { + // One address, so the flow that ends is the last holder of the one being printed. + public_ranges: vec![AddrInterval::new(BASE, BASE)], + reserved: PrefixPortsSet::new(), + idle_timeout: IDLE_TIMEOUT, + }]; + let pools = Arc::new(pool_sets_for_specs::( + &specs, + NextHeader::TCP, + false, + )); + + let allocation = pools[0].allocate(false).expect("the pool can serve"); + + let releaser = thread::spawn(move || drop(allocation)); + let printer = { + let pools = pools.clone(); + thread::spawn(move || { + let _ = format!("{}", pools[0]); + }) + }; + + printer.join().expect("the printing thread panicked"); + releaser.join().expect("the releasing thread panicked"); + }); +} diff --git a/nat/src/masquerade/apalloc/display.rs b/nat/src/masquerade/apalloc/display.rs index e3f5d205eb..8a63a57d3f 100644 --- a/nat/src/masquerade/apalloc/display.rs +++ b/nat/src/masquerade/apalloc/display.rs @@ -7,6 +7,7 @@ use super::alloc::{AllocatedIp, IpAllocator, NatPool, PoolSet}; use super::port_alloc::PortAllocator; use super::{NatAllocator, NatIp, NatIpWithBitmap, PoolTable, PoolTableKey}; use common::cliprovider::{CliSource, Heading}; +use concurrency::sync::{Arc, Weak}; use indenter::indented; use std::fmt::{Display, Error, Formatter, Result, Write}; @@ -90,8 +91,24 @@ where I: NatIpWithBitmap + Display, { fn fmt(&self, f: &mut Formatter<'_>) -> Result { - let pool = self.read(); - write!(f, "{pool}") + // The same hazard the allocation paths guard against, reached by printing the table. + // + // The pool holds weak references to the addresses in use; the strong ones belong to the + // blocks handed out from each. `NatPool`'s own `fmt` upgrades each weak reference to print + // it, and the guard below is held for all of that. Another thread ending the last flow on + // an address at that moment leaves one of those upgrades as the only strong reference, and + // letting it go runs `AllocatedIp::drop` here, which takes this same lock for writing. + // + // Holding an upgrade of every address across the guard keeps the ones taken while printing + // from ever being last. They are released below, once the guard is gone. + let mut examined: Vec>> = Vec::new(); + let outcome = { + let pool = self.read(); + examined.extend(pool.ips_in_use().filter_map(Weak::upgrade)); + write!(f, "{pool}") + }; + drop(examined); + outcome } } From 4a8cc4a2be82b92eb126293763fe4b0b972f385d Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 16:40:56 -0600 Subject: [PATCH 04/24] build(fuzz): Add just recipes for libfuzzer campaigns 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 [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-.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 Signed-off-by: Daniel Noland --- .gitignore | 3 ++ development/code/running-tests.md | 69 ++++++++++++++++++++++++++++++- justfile | 26 ++++++++++++ 3 files changed, 97 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 8450062587..2dbd36d349 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,8 @@ **.profraw **/__fuzz__/** +# libfuzzer writes one of these per worker into the working directory when +# `just fuzz` is given -j; the corpus itself lives under __fuzz__. +fuzz-*.log # qemu-user core dumps from SIGABRT under emulated tests. **/qemu_*.core result* diff --git a/development/code/running-tests.md b/development/code/running-tests.md index 697aa1e099..f15e435f45 100644 --- a/development/code/running-tests.md +++ b/development/code/running-tests.md @@ -62,8 +62,75 @@ change this. The major downside is that these processes are very computationally intensive and can take a long time to run. In fact, the [afl] fuzzer runs until you terminate it. +## Running a real fuzzing campaign + +To run a target under [libfuzzer], which is coverage guided and explores far deeper than the random +driver the test suite uses, list the targets and pick one: + +```shell +just fuzz-list -p dataplane-nat +just fuzz 'masquerade::apalloc::region::bolero_tests::decompose_properties' 10min -p dataplane-nat +``` + +The duration defaults to `60s`; anything after it is forwarded to `cargo bolero test`. As a sense of +the difference, a property that manages a few thousand cases per second under `just test` reaches +several hundred thousand per minute here, because libfuzzer mutates towards inputs that reach new +code rather than sampling blindly. + +Findings are written to a `__fuzz__` directory beside the test. That directory is gitignored: the +corpus is a local artifact that seeds later runs on the same machine, not something to commit. + +Pass `-j` to spread the campaign over more cores, which is the cheapest way to reach deeper: + +```shell +just fuzz 'some::module::tests::some_property' 10min -p some-package -j 60 +``` + +Each worker then writes a `fuzz-.log` into the directory you ran from, rather than into +`__fuzz__`. Those are gitignored too, and are only worth reading when a run reports a crash. + +### Sanitizers + +`cargo bolero` builds with the `fuzz` profile and links [AddressSanitizer] unless told otherwise, so +a plain `just fuzz` is already an asan campaign. To swap sanitizers, set the same `sanitize` +variable the rest of the justfile uses: + +```shell +just sanitize=thread fuzz 'some::module::tests::some_property' 5min -p some-package +``` + +[ThreadSanitizer] only reports on a target that actually spawns threads, so it is worth the extra +cost on a concurrency suite and close to pointless on a single-threaded property. It also takes +much longer to get going, because thread instrumentation changes the ABI: `just` therefore adds +`--build-std` for it, since a std left uninstrumented fails the build on a mismatch against `core`. + +A sanitizer is not free. Instrumentation costs roughly a factor of four in executions per second, +so it is worth spending some of a campaign with none at all, reaching deeper into the input space +in exchange for only catching what the test's own assertions catch: + +```shell +just sanitize=NONE fuzz 'some::module::tests::some_property' 30min -p some-package +``` + +The two are complementary: asan for memory errors the assertions cannot see, `NONE` for depth. + +The suite as a whole can also be run under either sanitizer with the standard runner, which is what +CI's `sanitize/fuzz/*` jobs do: + +```shell +just profile=fuzz sanitize=thread test +just profile=fuzz sanitize=address test +``` + +That covers far more code than a single fuzz target, but only with the brief random driver rather +than a real campaign. The two are complementary. + > [!NOTE] -> Dedicated `just` recipes for running full fuzz campaigns (with libfuzzer/afl) are planned for a future PR. +> `just fuzz` passes `--rustc-bootstrap`, because libfuzzer wants a nightly compiler for its +> sanitizer coverage flags while the pinned toolchain is stable. An [afl] recipe is still to come. + +[AddressSanitizer]: https://clang.llvm.org/docs/AddressSanitizer.html +[ThreadSanitizer]: https://clang.llvm.org/docs/ThreadSanitizer.html [README.md]: ../../README.md [afl]: https://github.com/AFLplusplus/AFLplusplus diff --git a/justfile b/justfile index e019461039..88244afb73 100644 --- a/justfile +++ b/justfile @@ -160,6 +160,32 @@ test package="tests.all" *args: (build (if package == "tests.all" { "tests.all" declare -r target="{{ if package == "tests.all" { "tests.all" } else { "tests.pkg." + package } }}" cargo nextest run --archive-file results/${target}/*.tar.zst --workspace-remap $(pwd) {{ filter }} +# List the bolero targets `just fuzz` can run. Args go to `cargo bolero list` +[script] +fuzz-list *args="": + {{ _just_debuggable_ }} + cargo bolero list {{ _cargo_feature_flags }} {{ args }} + +# Fuzz one bolero target under libfuzzer. See development/code/running-tests.md +[script] +fuzz target time="60s" *args="": + {{ _just_debuggable_ }} + # libfuzzer wants a nightly compiler for its sanitizer coverage flags, while the + # pinned toolchain is stable; --rustc-bootstrap bridges that. cargo-bolero already + # builds with the fuzz profile and links AddressSanitizer unless told otherwise, so + # a plain `just fuzz` is already an asan run. Findings land in a gitignored + # `__fuzz__` directory beside the test. + # + # `sanitize=thread` additionally rebuilds std: thread instrumentation changes the + # ABI, so a std left uninstrumented fails the build on a mismatch against `core`. + # asan does not need that, and skipping the std rebuild keeps it far quicker. + # `sanitize=NONE` drops instrumentation altogether, which buys roughly four times + # the executions per second in exchange for only catching what the test asserts. + cargo bolero test '{{ target }}' --rustc-bootstrap -T '{{ time }}' \ + {{ if sanitize != "" { "--sanitizer " + sanitize } else { "" } }} \ + {{ if sanitize == "thread" { "--build-std" } else { "" } }} \ + {{ _cargo_feature_flags }} {{ args }} + # Build and run the criterion benches. The rte_acl benches are gated behind the # `dpdk` feature, so run `just features=dpdk bench` to exercise them; a plain # `just bench` builds them as empty `main()` and only runs the reference benches. From 694ecfb9751c83f41dcc7fa2ba4b4a68a214a2ab Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 17:39:05 -0600 Subject: [PATCH 05/24] fix(masquerade): Only fall through to the next region on exhaustion 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/allocation.rs | 26 +++++++++++++++++++ nat/src/masquerade/apalloc/alloc.rs | 28 ++++++++++++++------ nat/src/masquerade/apalloc/pool_fuzz.rs | 34 +++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/nat/src/masquerade/allocation.rs b/nat/src/masquerade/allocation.rs index 4a5bed08d1..a9de28510d 100644 --- a/nat/src/masquerade/allocation.rs +++ b/nat/src/masquerade/allocation.rs @@ -35,6 +35,32 @@ pub enum AllocatorError { NoPoolFound, } +impl AllocatorError { + /// Whether this error says the space simply ran out, rather than that something is wrong. + /// + /// A caller holding several allocators over disjoint space may move on to the next one when + /// this holds, and only then: any other error is about the allocator rather than about how + /// full it is, and would be buried by a later success. The classification is the one + /// [`DoneReason`] already draws, where exactly these become `NatOutOfResources`. + /// The match is exhaustive on purpose: a new error has to be classified here rather than + /// silently defaulting to one side of it. + #[must_use] + pub fn is_exhaustion(&self) -> bool { + match self { + AllocatorError::NoFreeIp + | AllocatorError::NoPortBlock + | AllocatorError::NoFreePort(_) => true, + AllocatorError::PortAllocationFailed(_) + | AllocatorError::PortReservationFailed(_) + | AllocatorError::UnsupportedProtocol(_) + | AllocatorError::MissingDiscriminant + | AllocatorError::InternalIssue(_) + | AllocatorError::Denied + | AllocatorError::NoPoolFound => false, + } + } +} + impl From<&AllocatorError> for DoneReason { fn from(error: &AllocatorError) -> Self { match error { diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 42cbe8f675..457999d3fc 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -137,10 +137,17 @@ impl IpAllocator { // FIXME: Should we clean up every time?? self.cleanup_used_ips(); - if let Ok(port) = self.reuse_allocated_ip(allow_null) { - return Ok(port); + // 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: it walks past an address that + // has run out and reports anything else as it found it. Taking only `Ok` here would put + // that back, burying an error about the allocator under whatever the fresh address + // returns -- the same mistake `PoolSet::allocate` avoids one level up, where trying the + // next region on any error would bury it under a success. + match self.reuse_allocated_ip(allow_null) { + Ok(port) => Ok(port), + Err(e) if e.is_exhaustion() => self.allocate_from_new_ip(allow_null), + Err(e) => Err(e), } - self.allocate_from_new_ip(allow_null) } fn get_allocated_ip(&self, ip: I) -> Result>, AllocatorError> { @@ -227,21 +234,26 @@ impl PoolSet { /// Allocate from the first region with room. Regions are ordered so that those the expose has /// to itself are tried first, leaving shared space for exposes that have nowhere else to go. + /// + /// Only a region being full moves on to the next one. Any other error is about the allocator + /// rather than about how full that region is, and trying the next region would either bury it + /// under a success or replace it with a later region's `NoFreeIp`. pub(crate) fn allocate( &self, allow_null: bool, ) -> Result, AllocatorError> { - let mut last_error = None; + let mut exhausted = None; for region in &self.regions { match region.allocator.allocate(allow_null) { Ok(port) => return Ok(port), - Err(e) => { - debug!("Region {:?} could not allocate: {e}", region.range); - last_error = Some(e); + Err(e) if e.is_exhaustion() => { + debug!("Region {:?} is out of space: {e}", region.range); + exhausted = Some(e); } + Err(e) => return Err(e), } } - Err(last_error.unwrap_or(AllocatorError::NoFreeIp)) + Err(exhausted.unwrap_or(AllocatorError::NoFreeIp)) } /// Reserve a specific address and port, which has to come from the region owning that address. diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 15da6c3feb..cd8a91fcd6 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -258,6 +258,40 @@ fn re_reservation_after_a_config_change_is_honoured() { }); } +/// Falling through to the next region is what makes an expose's several regions behave as one +/// pool. Only exhaustion may do it: any other error is about the allocator rather than about how +/// full a region is, and a later region's success would bury it. +/// +/// Exhausting a region by allocating from it would take every port of every address it holds, so +/// this reserves them instead, which reaches the same state in one step. +#[test] +fn an_exhausted_region_falls_through_to_the_next() { + // Not adjacent, or the two would merge into a single region. + let full = BASE; + let free = BASE + 4; + + let every_port = PrefixWithOptionalPorts::new( + "10.1.0.0/32".into(), + Some(PortRange::new(1024, u16::MAX).unwrap_or_else(|_| unreachable!())), + ); + + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(full, full), AddrInterval::new(free, free)], + reserved: [every_port].into_iter().collect(), + idle_timeout: IDLE_TIMEOUT, + }]; + + let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let allocation = pool_sets[0] + .allocate(false) + .expect("the second region has room, so allocation must succeed"); + assert_eq!( + allocation.ip(), + Ipv4Addr::from(u32::try_from(free).unwrap_or_else(|_| unreachable!())), + "allocation did not fall through to the region with room" + ); +} + /////////////////////////////////////////////////////////////////////////////// // Reserved ports /////////////////////////////////////////////////////////////////////////////// From c86a190040be85f069d4e5d236e9f7a20b34543d Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 17:41:25 -0600 Subject: [PATCH 06/24] fix(masquerade): Refuse an unmappable address instead of panicking 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 33 ++++++-- .../masquerade/apalloc/natip_with_bitmap.rs | 2 +- nat/src/masquerade/apalloc/pool_fuzz.rs | 75 ++++++++++++++++++- 3 files changed, 102 insertions(+), 8 deletions(-) diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 457999d3fc..42e796cadd 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -22,7 +22,7 @@ use roaring::RoaringBitmap; use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::net::{IpAddr, Ipv6Addr}; use std::time::Duration; -use tracing::debug; +use tracing::{debug, error}; /////////////////////////////////////////////////////////////////////////////// // IpAllocator @@ -467,8 +467,15 @@ impl NatPool { fn deallocate_from_pool(&mut self, ip: I) { debug!("Address {ip} was deallocated"); - let offset = I::try_to_offset(ip, &self.reverse_bitmap_mapping).unwrap(); - self.bitmap.set_ip_free(offset); + // The address was handed out by this pool, so it maps back into it. This runs while an + // allocation is being dropped and has nowhere to report a failure, so say so and leave the + // address marked in use rather than panicking on the drop path. + match I::try_to_offset(ip, &self.reverse_bitmap_mapping) { + Ok(offset) => { + self.bitmap.set_ip_free(offset); + } + Err(e) => error!("Address {ip} does not map back into the pool it came from: {e}"), + } } /// `keep_alive` collects every address upgraded here, for the caller to release once the pool @@ -626,11 +633,25 @@ pub(crate) fn map_offset( } // Reverse operation from map_offset() -pub(crate) fn map_address(address: Ipv6Addr, bitmap_mapping: &BTreeMap) -> u32 { +// +// Not every address of a region has an offset. A region may hold more addresses than a u32 can +// index, in which case the bitmap covers only the first 2^32 of them (see `NatPool::for_range`), +// and an address beyond that is simply not one this pool can serve. That is reachable from +// configuration rather than a bug: a flow carried across a config change presents the address it +// already holds, and the region it falls in may have grown downwards underneath it. Report it as +// the pool not serving the address, so the flow is dropped like any other that cannot be carried +// over, rather than panicking in the middle of applying a config. +pub(crate) fn map_address( + address: Ipv6Addr, + bitmap_mapping: &BTreeMap, +) -> Result { let (prefix_start_bits, prefix_offset) = bitmap_mapping .range(..=address.to_bits()) .next_back() - .expect("This should never fail"); + .ok_or(AllocatorError::NoPoolFound)?; - prefix_offset + u32::try_from(address.to_bits() - prefix_start_bits).unwrap() + u32::try_from(address.to_bits() - prefix_start_bits) + .ok() + .and_then(|offset| prefix_offset.checked_add(offset)) + .ok_or(AllocatorError::NoPoolFound) } diff --git a/nat/src/masquerade/apalloc/natip_with_bitmap.rs b/nat/src/masquerade/apalloc/natip_with_bitmap.rs index 52385055a7..73e8c04703 100644 --- a/nat/src/masquerade/apalloc/natip_with_bitmap.rs +++ b/nat/src/masquerade/apalloc/natip_with_bitmap.rs @@ -60,6 +60,6 @@ impl NatIpWithBitmap for Ipv6Addr { bitmap_mapping: &BTreeMap, ) -> Result { // Reverse operation of map_offset() - Ok(map_address(address, bitmap_mapping)) + map_address(address, bitmap_mapping) } } diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index cd8a91fcd6..0fd4d5cbc0 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -23,11 +23,12 @@ use super::alloc::PoolSet; use super::region::AddrInterval; use super::setup::{PoolSpec, pool_sets_for_specs}; use crate::masquerade::allocation::AllocatorError; +use crate::port::NatPort; use bolero::{Driver, TypeGenerator}; use lpm::prefix::{PortRange, PrefixPortsSet, PrefixWithOptionalPorts}; use net::ip::NextHeader; use std::collections::BTreeSet; -use std::net::Ipv4Addr; +use std::net::{Ipv4Addr, Ipv6Addr}; use std::time::Duration; // 10.1.0.0, with a window small enough that regions stay cheap to build. @@ -292,6 +293,78 @@ fn an_exhausted_region_falls_through_to_the_next() { ); } +/////////////////////////////////////////////////////////////////////////////// +// IPv6 +/////////////////////////////////////////////////////////////////////////////// + +/// A region may hold more addresses than a `u32` can index, in which case the bitmap covers only +/// the first 2^32 of them. An address past that is inside the region but not servable by the pool. +/// +/// A flow carried across a config change is the way to present one: it holds the address it was +/// already given, and the region it falls in may have grown downwards underneath it. That has to +/// come back as an error, so the flow is dropped like any other that cannot be carried over, +/// rather than panicking part way through applying a config. +#[test] +fn an_address_past_the_indexable_span_is_refused_rather_than_panicking() { + let start = u128::from(Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 0)); + // Far wider than the bitmap can index. + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(start, start + (1u128 << 40))], + reserved: PrefixPortsSet::new(), + idle_timeout: IDLE_TIMEOUT, + }]; + let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let port = NatPort::new_port_checked(4096).unwrap_or_else(|_| unreachable!()); + + // Just inside the indexable span: an ordinary carry-over, which must still work. + let near = Ipv6Addr::from(start + 1); + assert!( + pool_sets[0].reserve(near, port).is_ok(), + "an address the pool can index was refused" + ); + + // Past it: refused, and specifically not a panic. + let far = Ipv6Addr::from(start + (1u128 << 33)); + assert_eq!( + pool_sets[0].reserve(far, port).unwrap_err(), + AllocatorError::NoPoolFound, + "an address the pool cannot index should be refused as unserved" + ); +} + +/// The pools are generic over the address family but every other test here is IPv4. Allocating +/// from an IPv6 pool goes through the offset mapping that IPv4 skips entirely, so cover it. +#[test] +fn ipv6_pools_allocate_within_their_range() { + let start = u128::from(Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 0)); + let end = start + 3; + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(start, end)], + reserved: PrefixPortsSet::new(), + idle_timeout: IDLE_TIMEOUT, + }]; + let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + + let mut held = Vec::new(); + let mut seen = BTreeSet::new(); + for _ in 0..8 { + let allocation = pool_sets[0].allocate(false).expect("pool has room"); + let bits = u128::from(allocation.ip()); + assert!( + (start..=end).contains(&bits), + "{} is outside the range the pool was built for", + allocation.ip() + ); + assert!( + seen.insert((allocation.ip(), allocation.port().as_u16())), + "{}:{} was handed out twice", + allocation.ip(), + allocation.port() + ); + held.push(allocation); + } +} + /////////////////////////////////////////////////////////////////////////////// // Reserved ports /////////////////////////////////////////////////////////////////////////////// From cc136fc05d258998517c0b759b48f58d6607c34c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 17:45:34 -0600 Subject: [PATCH 07/24] test(masquerade): Close the gap in the concurrent uniqueness oracle 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 105 ++++++++++++------ 1 file changed, 69 insertions(+), 36 deletions(-) diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs index f5ebaa494c..2692a68ed5 100644 --- a/nat/src/masquerade/apalloc/concurrent_fuzz.rs +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -28,7 +28,9 @@ //! * A published allocator never hands out an address and port that was carried over into it. This //! is the safety property of the update: the writer re-reserves before publishing, so a flow that //! survived a config change and a flow created just after it must not collide on the reverse key. -//! * An address and port is never handed to two live flows drawn from the same allocator. +//! * An address and port is never handed to two live flows drawn from the same allocator. Shapes +//! that hold every allocation for the length of the run check this exactly; those that free as +//! they go trade that for exercising deallocation. See [`Live`] for why the two differ. //! * Neither allocation nor reservation ever reports [`AllocatorError::InternalIssue`]. That is the //! allocator saying its own bookkeeping is inconsistent, and `find_block_for_port` carries a //! standing `FIXME` wondering whether the block it just found non-free can be released before it @@ -110,6 +112,12 @@ struct Scenario { ranges: Vec>, packet_ops: [Vec; PACKET_WORKERS], config_ops: Vec, + /// Whether flows may end while the run is in progress. + /// + /// Freeing is worth exercising, because it is what returns an address to a pool, but it costs + /// the uniqueness oracle its certainty: see [`Live`]. Half the shapes therefore hold every + /// allocation for the whole run, which makes the record monotone and the oracle exact. + frees_allowed: bool, } impl bolero::TypeGenerator for Scenario { @@ -152,6 +160,7 @@ impl bolero::TypeGenerator for Scenario { ranges, packet_ops, config_ops, + frees_allowed: driver.produce()?, }) } } @@ -211,6 +220,25 @@ impl Published { /// per-thread record would not see it. Two allocations from *different* generations may legitimately /// repeat: only the survivors carried into a new generation are protected there, which is what /// [`Published::carried`] covers. +/// +/// # What this catches, and what it cannot +/// +/// The record is written after the allocator has already handed a pair out, so it observes +/// allocation rather than being part of it. That is what keeps the threads racing on the +/// allocator's own locks instead of on this mutex, and it costs the oracle something once a pair +/// 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 goes unseen. That +/// interleaving is *indistinguishable* from one thread legitimately reusing what another gave +/// back, so no record kept at these two points can tell them apart. +/// +/// The way out is to leave nothing to give back: when [`Scenario::frees_allowed`] is false no +/// allocation is released for the length of the run, the record only grows, and a duplicate is +/// caught with certainty. Roughly half the generated shapes are of that kind. The rest trade that +/// certainty for exercising deallocation, and still catch every duplicate whose holders overlap in +/// the record, which is the common case. +/// +/// Closing the gap outright would take instrumenting the allocator itself, so that a pair is +/// recorded as part of being handed out rather than just after. struct Live(Mutex>); impl Live { @@ -218,9 +246,7 @@ impl Live { Self(Mutex::new(BTreeSet::new())) } - /// Record a freshly allocated pair. The allocation already exists by the time this runs, so a - /// thread that raced us to the same pair has either recorded it already or is about to fail - /// here itself; either way the collision is caught. + /// Record a freshly allocated pair, failing if it is already held. fn claim(&self, generation: u64, ip: Ipv4Addr, port: u16) { let mut live = self.0.lock(); assert!( @@ -279,64 +305,75 @@ impl Scenario { let live = Arc::new(Live::new()); thread::scope(|scope| { - let mut handles = Vec::new(); + let mut packet_handles = Vec::new(); for (index, ops) in self.packet_ops.iter().enumerate() { let slot = slot.clone(); let live = live.clone(); let ops = ops.clone(); let survivors = survivors.clone(); - handles.push( + let frees_allowed = self.frees_allowed; + packet_handles.push( thread::Builder::new() .name(format!("packet-{index}")) .spawn_scoped(scope, move || { - packet_worker(&slot, &live, &ops, &survivors); + packet_worker(&slot, &live, &ops, &survivors, frees_allowed) }) .expect("spawn packet worker"), ); } - { + let config_handle = { let slot = slot.clone(); let ops = self.config_ops.clone(); let specs = specs.clone(); let survivors = survivors.clone(); - handles.push( - thread::Builder::new() - .name("config".to_string()) - .spawn_scoped(scope, move || { - let mut generation = 0u64; - for op in ops { - match op { - ConfigOp::Republish => { - generation += 1; - let next = Published::build(&specs, generation, &survivors); - slot.store(Some(Arc::new(next))); - } - ConfigOp::Idle => {} + thread::Builder::new() + .name("config".to_string()) + .spawn_scoped(scope, move || { + let mut generation = 0u64; + for op in ops { + match op { + ConfigOp::Republish => { + generation += 1; + let next = Published::build(&specs, generation, &survivors); + slot.store(Some(Arc::new(next))); } - thread::yield_now(); + ConfigOp::Idle => {} } - }) - .expect("spawn config worker"), - ); - } + thread::yield_now(); + } + }) + .expect("spawn config worker") + }; - for handle in handles { - handle.join().expect("worker panicked"); - } + // Collect rather than drop: an allocation released while another thread is still + // allocating is one the record cannot reason about, so everything stays held until + // every thread has finished. + let leftovers: Vec<_> = packet_handles + .into_iter() + .map(|handle| handle.join().expect("packet worker panicked")) + .collect(); + config_handle.join().expect("config worker panicked"); + drop(leftovers); }); drop(existing); } } +/// Returns whatever the thread is still holding when its ops run out, rather than releasing it. +/// +/// Releasing here would free addresses while other threads are still allocating, which is exactly +/// the ambiguity [`Live`] cannot see through, and it is not needed to keep the record honest: the +/// pairs stay held, so nothing else can legitimately be given them. fn packet_worker( slot: &SlotOption, live: &Live, ops: &[PacketOp], survivors: &[(usize, Ipv4Addr, NatPort)], -) { + frees_allowed: bool, +) -> Vec<(u64, AllocatedPort)> { // What this thread is holding, tagged with the generation it was drawn from. let mut held: Vec<(u64, AllocatedPort)> = Vec::new(); @@ -374,7 +411,7 @@ fn packet_worker( } } PacketOp::FreeOldest => { - if !held.is_empty() { + if frees_allowed && !held.is_empty() { let (generation, allocation) = held.remove(0); live.release(generation, allocation); } @@ -399,11 +436,7 @@ fn packet_worker( thread::yield_now(); } - // Give everything back through the record, so a thread that finishes early cannot leave - // entries behind and make a later, legitimate allocation look like a collision. - for (generation, allocation) in held { - live.release(generation, allocation); - } + held } #[concurrency::model_test] From 090a2f7d289b7fd57e04ae5754f050254225a3a8 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 15:45:00 -0600 Subject: [PATCH 08/24] fix(masquerade): Make an allocation a lease only one thing can hold 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 2 +- nat/src/masquerade/apalloc/mod.rs | 2 +- nat/src/masquerade/apalloc/port_alloc.rs | 13 ++++++-- nat/src/masquerade/state.rs | 2 +- nat/src/masquerade/test.rs | 40 ++++++++++++++---------- 5 files changed, 36 insertions(+), 23 deletions(-) diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 42e796cadd..004cee6138 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -360,7 +360,7 @@ impl Drop for AllocatedIp { /// A [`NatPool`] is a pool of IP addresses that can be allocated from. It contains a bitmap of /// available IP addresses, and a list of weak references to [`AllocatedIp`] objects representing /// the allocated IPs potentially available for use (if they still have free ports) -#[derive(Debug, Clone)] +#[derive(Debug)] pub(crate) struct NatPool { bitmap: PoolBitmap, bitmap_mapping: BTreeMap, diff --git a/nat/src/masquerade/apalloc/mod.rs b/nat/src/masquerade/apalloc/mod.rs index d778a182c2..4d33677244 100644 --- a/nat/src/masquerade/apalloc/mod.rs +++ b/nat/src/masquerade/apalloc/mod.rs @@ -203,7 +203,7 @@ impl PoolTable { /////////////////////////////////////////////////////////////////////////////// /// [`Allocation`] is the non-generic object representing an allocation, be it IPv4 or IPv6 -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum Allocation { V4(AllocatedPort), V6(AllocatedPort), diff --git a/nat/src/masquerade/apalloc/port_alloc.rs b/nat/src/masquerade/apalloc/port_alloc.rs index e97be82bf7..a5bf1cd4ab 100644 --- a/nat/src/masquerade/apalloc/port_alloc.rs +++ b/nat/src/masquerade/apalloc/port_alloc.rs @@ -21,7 +21,7 @@ use lpm::prefix::PortRange; use std::collections::{BTreeSet, HashMap}; use std::fmt::Display; -use tracing::debug; +use tracing::{debug, error}; #[concurrency_mode(std)] use rand::seq::SliceRandom; @@ -576,7 +576,7 @@ impl Drop for AllocatedPortBlock { /// /// It contains a back reference to its parent [`AllocatedPortBlock`], to deallocate the port when /// the [`AllocatedPort`] is dropped. -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct AllocatedPort { port: NatPort, // the actual allocated value block_allocator: Arc>, // block/IP the allocated value belongs to @@ -612,7 +612,14 @@ impl AllocatedPort { impl Drop for AllocatedPort { fn drop(&mut self) { debug!("Dropping allocated port {self}..."); - let _ = self.block_allocator.deallocate_port_from_block(self.port); + // Not panicking on a drop path is right; discarding the answer is not. A port that cannot + // be given back has either been given back already or was never recorded as taken, and + // both mean the bitmap no longer says what is in use -- the same accounting whose silence + // let a pair be handed out twice until the error above was made load-bearing. There is + // nothing to do about it here, but it should not pass unsaid. + if let Err(e) = self.block_allocator.deallocate_port_from_block(self.port) { + error!("Failed to give back {self}: {e}"); + } } } diff --git a/nat/src/masquerade/state.rs b/nat/src/masquerade/state.rs index 17cc8bfbfc..43df798490 100644 --- a/nat/src/masquerade/state.rs +++ b/nat/src/masquerade/state.rs @@ -9,7 +9,7 @@ use std::fmt::Display; use std::net::IpAddr; use std::time::Duration; -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct MasqueradeState { pub(crate) status: AtomicNatFlowStatus, action: NatAction, diff --git a/nat/src/masquerade/test.rs b/nat/src/masquerade/test.rs index 962b5f5a3b..2fcd119035 100644 --- a/nat/src/masquerade/test.rs +++ b/nat/src/masquerade/test.rs @@ -1420,17 +1420,20 @@ fn nat_flow_status(packet: &Packet) -> Option { .map(|state| state.status.load()) } -fn masquerade_state(packet: &Packet) -> Option { - packet - .meta() - .flow_info - .as_ref()? - .locked - .read() +// Read something out of a flow's masquerade state, under the lock. +// +// Deliberately not a clone of the state: it owns the allocation, and a copy of it is a second +// owner of the same public address and port. Nothing in a test needs that. +fn with_masquerade_state( + packet: &Packet, + read: impl FnOnce(&MasqueradeState) -> T, +) -> Option { + let locked = packet.meta().flow_info.as_ref()?.locked.read(); + let state = locked .nat_state - .as_ref() - .and_then(|s| s.extract_ref::()) - .cloned() + .as_ref()? + .extract_ref::()?; + Some(read(state)) } fn build_reply(packet: &Packet) -> Packet { @@ -1502,7 +1505,7 @@ fn establish_tcp_connection(pipeline: &mut DynPipeline) { assert_eq!(nat_flow_status(&output), Some(NatFlowStatus::Established)); // configured timeout for the flow - let timeout = masquerade_state(&output).unwrap().idle_timeout(); + let timeout = with_masquerade_state(&output, MasqueradeState::idle_timeout).unwrap(); // check that flow timeouts "match" the ones configured, allowing for 5 second error (for the test) let flow_info_ack = output.meta().flow_info.as_ref().unwrap(); @@ -1539,8 +1542,9 @@ async fn test_masquerade_check() { let out = process_packet(&mut pipeline, packet); // packet hit flow with SRC nat rule - let state = masquerade_state(&out).expect("Must have flow info w/ masquerade state"); - assert_eq!(state.action(), NatAction::SrcNat); + let action = with_masquerade_state(&out, MasqueradeState::action) + .expect("Must have flow info w/ masquerade state"); + assert_eq!(action, NatAction::SrcNat); test_case("Process packet masquerade dest nat"); // process packet in dst nat direction @@ -1548,8 +1552,9 @@ async fn test_masquerade_check() { let out = process_packet(&mut pipeline, reply); // packet hit flow with dst nat rule - let state = masquerade_state(&out).expect("Must have flow info w/ masquerade state"); - assert_eq!(state.action(), NatAction::DstNat); + let action = with_masquerade_state(&out, MasqueradeState::action) + .expect("Must have flow info w/ masquerade state"); + assert_eq!(action, NatAction::DstNat); assert_eq!(nat_flow_status(&out).unwrap(), NatFlowStatus::Established); assert_eq!(flow_status(&out).unwrap(), FlowStatus::Active); @@ -1582,8 +1587,9 @@ async fn test_masquerade_tcp_reset() { let reply_out = process_packet(&mut pipeline, reply); // packet hits flow with dst nat rule. Nat flow status becomes reset and flow is cancelled - let state = masquerade_state(&reply_out).expect("Must have flow info w/ masquerade state"); - assert_eq!(state.action(), NatAction::DstNat); + let action = with_masquerade_state(&reply_out, MasqueradeState::action) + .expect("Must have flow info w/ masquerade state"); + assert_eq!(action, NatAction::DstNat); assert_eq!(nat_flow_status(&out).unwrap(), NatFlowStatus::Reset); assert_eq!(flow_status(&reply_out).unwrap(), FlowStatus::Cancelled); From 2ae8334744b90d85388a50a2c8e21b6470a17e36 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 21:44:22 -0600 Subject: [PATCH 09/24] fix(masquerade): Stretch the flow timeouts when the tests run emulated 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/nf.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/nat/src/masquerade/nf.rs b/nat/src/masquerade/nf.rs index 7eecaa3552..0acb9a802e 100644 --- a/nat/src/masquerade/nf.rs +++ b/nat/src/masquerade/nf.rs @@ -69,10 +69,26 @@ pub struct Masquerade { } impl Masquerade { + // How far the flow timeouts below are stretched when the tests are run under an emulator. + // + // A flow expires against the wall clock, while a test refreshes it by doing work: sending a + // packet. Under miri or qemu-user that work takes something like two orders of magnitude + // longer, 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, and the test fails on a clock rather than on anything it meant to + // check. Stretching the timeouts keeps them measuring the same amount of work either way. + // + // `emulated` is set only by the miri and qemu-user paths, so a real data plane always gets the + // native values. + const TIMEOUT_SCALE: u64 = cfg_select! { + emulated => 100, + _ => 1, + }; + // Internal flow timeouts for masquerading - pub const MASQUERADE_ONEWAY_TIMEOUT: Duration = Duration::from_secs(5); - pub const MASQUERADE_TWOWAY_TIMEOUT: Duration = Duration::from_secs(3); - pub const MASQUERADE_CLOSING_TIMEOUT: Duration = Duration::from_secs(2); + pub const MASQUERADE_ONEWAY_TIMEOUT: Duration = Duration::from_secs(5 * Self::TIMEOUT_SCALE); + pub const MASQUERADE_TWOWAY_TIMEOUT: Duration = Duration::from_secs(3 * Self::TIMEOUT_SCALE); + pub const MASQUERADE_CLOSING_TIMEOUT: Duration = Duration::from_secs(2 * Self::TIMEOUT_SCALE); /// Creates a new [`Masquerade`] processor from provided parameters. #[must_use] From 2efa89011c1c44fdb44593932ad39af8d9bc9c74 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 18:29:07 -0600 Subject: [PATCH 10/24] test(masquerade): Cover two VPCs sharing a private prefix, end to end 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/test.rs | 127 +++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/nat/src/masquerade/test.rs b/nat/src/masquerade/test.rs index 2fcd119035..7533c02fe9 100644 --- a/nat/src/masquerade/test.rs +++ b/nat/src/masquerade/test.rs @@ -329,6 +329,69 @@ fn build_overlay_2vpcs_modified() -> Overlay { Overlay::new(vpc_table, peering_table) } +// Two source VPCs using the *same* private prefix, both masquerading towards VPC-3 and each onto +// a public range of its own. Tenants reusing private address space is ordinary, and is much of +// what NAT is for, so a private address only identifies a pool together with the VPC it belongs +// to. +fn build_overlay_shared_private_prefix() -> Overlay { + let mut vpc_table = VpcTable::new(); + let _ = vpc_table.add(Vpc::new("VPC-1", "AAAAA", 100).expect("Failed to add VPC")); + let _ = vpc_table.add(Vpc::new("VPC-2", "BBBBB", 200).expect("Failed to add VPC")); + let _ = vpc_table.add(Vpc::new("VPC-3", "CCCCC", 300).expect("Failed to add VPC")); + + let expose13 = VpcExpose::empty() + .make_masquerade(None) + .unwrap() + .ip("1.1.0.0/16".into()) + .as_range("2.2.0.0/16".into()) + .unwrap(); + // The same private space as VPC-1, onto a different public range. + let expose23 = VpcExpose::empty() + .make_masquerade(None) + .unwrap() + .ip("1.1.0.0/16".into()) + .as_range("4.4.0.0/16".into()) + .unwrap(); + + let peering13 = VpcPeering::with_default_group( + "VPC-1--VPC-3", + VpcManifest::new("VPC-1").exposing(expose13), + VpcManifest::new("VPC-3").exposing(VpcExpose::empty().ip("3.3.3.0/24".into())), + ); + let peering23 = VpcPeering::with_default_group( + "VPC-2--VPC-3", + VpcManifest::new("VPC-2").exposing(expose23), + VpcManifest::new("VPC-3").exposing(VpcExpose::empty().ip("3.3.3.0/24".into())), + ); + + let mut peering_table = VpcPeeringTable::new(); + peering_table.add(peering13).expect("Failed to add peering"); + peering_table.add(peering23).expect("Failed to add peering"); + + Overlay::new(vpc_table, peering_table) +} + +// A TCP packet towards VPC-3, from a given source VPC and private address. A SYN opens a flow; +// anything else is only translated if one already exists. +fn tcp_from(src_vni_id: u32, src_ip: &str, syn: bool) -> Packet { + let mut packet = build_test_tcp_ipv4_packet(src_ip, "3.3.3.1", 4321, 80); + { + let tcp = packet.try_tcp_mut().unwrap(); + tcp.set_syn(syn); + tcp.set_ack(false); + tcp.set_fin(false); + tcp.set_rst(false); + } + packet.meta_mut().set_overlay(true); + packet.meta_mut().src_vpcd = Some(vpcd(src_vni_id)); + packet.meta_mut().set_masquerade(true); + packet +} + +fn translated_source(packet: &Packet) -> Ipv4Addr { + packet.try_ipv4().unwrap().source().inner() +} + fn check_packet( nat: &mut Masquerade, src_vni: Vni, @@ -1640,6 +1703,70 @@ async fn test_masquerade_reconfig_keep_flow() { assert_eq!(flow_table.active_len(), Some(2)); } +// Two VPCs using the same private address, masquerading towards the same peer. Each has to be +// given its own public range, and has to keep it across a config change: re-reservation looks the +// pool up by source VPC as well, so a flow carried over must land back in the pool its own expose +// describes rather than in the other VPC's. +#[tokio::test] +#[cfg_attr(not(emulated), traced_test)] +async fn test_masquerade_reconfig_two_vpcs_sharing_a_private_prefix() { + let genid = 1; + let (flow_table, mut pipeline, mut allocw) = + test_setup(genid, &build_overlay_shared_private_prefix()); + + // The same private source address, in two different VPCs. The SYN opens the flow; the packet + // that creates a flow does not carry it, so a follow-up packet is what shows the flow state. + process_packet(&mut pipeline, tcp_from(100, "1.1.0.1", true)); + process_packet(&mut pipeline, tcp_from(200, "1.1.0.1", true)); + + let from_vpc1 = process_packet(&mut pipeline, tcp_from(100, "1.1.0.1", false)); + let from_vpc2 = process_packet(&mut pipeline, tcp_from(200, "1.1.0.1", false)); + + let public1 = translated_source(&from_vpc1); + let public2 = translated_source(&from_vpc2); + assert_eq!( + public1.octets()[0..2], + [2, 2], + "VPC-1 was not masqueraded onto the range its own expose declares, got {public1}" + ); + assert_eq!( + public2.octets()[0..2], + [4, 4], + "VPC-2 was not masqueraded onto the range its own expose declares, got {public2}" + ); + assert_eq!(flow_genid(&from_vpc1).unwrap(), genid); + assert_eq!(flow_genid(&from_vpc2).unwrap(), genid); + + // Apply an identical config. Nothing is re-reserved: `MasqueradeConfig`'s equality leaves + // genid out, so the writer keeps the allocator it has and only upgrades the flows onto the new + // genid. What this pins is that the two flows come through that untouched, still told apart by + // source VPC. Carrying flows into a *new* allocator is a different path, and reaching it takes + // a config that actually differs. + let overlay = build_overlay_shared_private_prefix().validate().unwrap(); + let nat_config = MasqueradeConfig::new(overlay.vpc_table(), genid + 1); + allocw.update_nat_allocator(nat_config, &flow_table); + + // Both survive, each still translated exactly as before. + let from_vpc1 = process_packet(&mut pipeline, tcp_from(100, "1.1.0.1", false)); + let from_vpc2 = process_packet(&mut pipeline, tcp_from(200, "1.1.0.1", false)); + + assert_eq!( + translated_source(&from_vpc1), + public1, + "VPC-1's flow did not keep its public address across the config change" + ); + assert_eq!( + translated_source(&from_vpc2), + public2, + "VPC-2's flow did not keep its public address across the config change" + ); + assert_eq!(flow_genid(&from_vpc1).unwrap(), genid + 1); + assert_eq!(flow_genid(&from_vpc2).unwrap(), genid + 1); + + tokio::time::sleep(Duration::from_secs(1)).await; + assert_eq!(flow_table.active_len(), Some(4)); +} + #[tokio::test] #[cfg_attr(not(emulated), traced_test)] async fn test_masquerade_reconfig_drop_flow() { From 4cf0eae15d1f1e3c641ab447021c399cda49ce5e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 18:35:53 -0600 Subject: [PATCH 11/24] fix(masquerade): Honour every port-forwarding claim on an address 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 41 +++-- nat/src/masquerade/apalloc/display.rs | 10 +- nat/src/masquerade/apalloc/mod.rs | 1 + nat/src/masquerade/apalloc/pool_fuzz.rs | 30 +--- nat/src/masquerade/apalloc/port_alloc.rs | 112 ++++++------- nat/src/masquerade/apalloc/reserved.rs | 191 +++++++++++++++++++++++ nat/src/masquerade/apalloc/setup.rs | 22 ++- nat/src/ranges.rs | 1 - 8 files changed, 286 insertions(+), 122 deletions(-) create mode 100644 nat/src/masquerade/apalloc/reserved.rs diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 004cee6138..9d983523ad 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -10,6 +10,7 @@ //! See also the architecture diagram at the top of mod.rs. use super::region::AddrInterval; +use super::reserved::{PortClaims, ReservedPorts}; use super::{NatIpWithBitmap, port_alloc}; use crate::masquerade::allocation::AllocatorError; use crate::masquerade::natip::NatIp; @@ -17,7 +18,6 @@ use crate::port::NatPort; use crate::ranges::IpRange; use concurrency::sync::{Arc, RwLock, RwLockReadGuard, Weak}; use lpm::prefix::PortRange; -use lpm::prefix::range_map::DisjointRangesBTreeMap; use roaring::RoaringBitmap; use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::net::{IpAddr, Ipv6Addr}; @@ -291,14 +291,14 @@ impl AllocatedIp { fn new( ip: I, ip_allocator: IpAllocator, - reserved_port_range: Option, + reserved_ports: PortClaims, randomize: bool, exclude_wellknown_ports: bool, ) -> Self { Self { ip, port_allocator: port_alloc::PortAllocator::new( - reserved_port_range, + reserved_ports, randomize, exclude_wellknown_ports, ), @@ -366,7 +366,7 @@ pub(crate) struct NatPool { bitmap_mapping: BTreeMap, reverse_bitmap_mapping: BTreeMap, in_use: VecDeque>>, - reserved_prefixes_ports: Option>, + reserved_ports: ReservedPorts, exclude_wellknown_ports: bool, } @@ -377,7 +377,7 @@ impl NatPool { /// overlap and a public address may only be handed out by one pool. pub(crate) fn for_range( range: AddrInterval, - reserved_prefixes_ports: Option>, + reserved_ports: ReservedPorts, exclude_wellknown_ports: bool, ) -> Self { // Index the region from its own start. IPv4 indexes its bitmap by the address bits and @@ -403,7 +403,7 @@ impl NatPool { bitmap_mapping, reverse_bitmap_mapping, in_use: VecDeque::new(), - reserved_prefixes_ports, + reserved_ports, exclude_wellknown_ports, } } @@ -429,15 +429,11 @@ impl NatPool { } // Used for Display - pub(crate) fn reserved_prefixes_ports( - &self, - ) -> Option> { - Some( - self.reserved_prefixes_ports - .as_ref()? - .iter() - .map(|(&r, &p)| (r, p)), - ) + pub(crate) fn reserved_ports(&self) -> Option> { + if self.reserved_ports.is_empty() { + return None; + } + Some(self.reserved_ports.iter()) } fn use_new_ip( @@ -450,16 +446,13 @@ impl NatPool { let ip = I::try_from_offset(offset, &self.bitmap_mapping)?; - // Check if the IP is in a reserved prefix, retrieve the reserved port range if any - let reserved_port_range = self - .reserved_prefixes_ports - .as_ref() - .and_then(|ranges| ranges.lookup(&ip.to_ip_addr()).map(|(_, range)| *range)); + // Every port range port forwarding has claimed on this address, not just one of them. + let claims = self.reserved_ports.for_address(ip.to_ip_addr()); Ok(AllocatedIp::new( ip, ip_allocator, - reserved_port_range, + claims, randomize, self.exclude_wellknown_ports, )) @@ -512,7 +505,11 @@ impl NatPool { let arc_ip = Arc::new(AllocatedIp::new( ip, ip_allocator, - None, + // An address entering the pool this way serves later allocations exactly as one that + // arrived through allocate(), so it has to carry the same claims. Passing none here + // let a flow carried across a config change bring an address in unencumbered, after + // which masquerade could hand out the ports port forwarding had taken on it. + self.reserved_ports.for_address(ip.to_ip_addr()), randomize, // Keep the low-port exclusion policy for explicitly reserved IPs as well, so // reserve() follows the same TCP/UDP allocation rules as allocate(). diff --git a/nat/src/masquerade/apalloc/display.rs b/nat/src/masquerade/apalloc/display.rs index 8a63a57d3f..98f1135f8f 100644 --- a/nat/src/masquerade/apalloc/display.rs +++ b/nat/src/masquerade/apalloc/display.rs @@ -117,7 +117,7 @@ where I: NatIpWithBitmap + Display, { fn fmt(&self, f: &mut Formatter<'_>) -> Result { - if let Some(reserved) = self.reserved_prefixes_ports() { + if let Some(reserved) = self.reserved_ports() { writeln!(f, "reserved ranges:")?; for (ips, ports) in reserved { writeln!(with_indent!(f), "{ips}:{ports}")?; @@ -164,8 +164,12 @@ where I: NatIpWithBitmap + Display, { fn fmt(&self, f: &mut Formatter<'_>) -> Result { - if let Some(reserved) = self.reserved_port_range() { - writeln!(f, "reserved port range: {reserved}")?; + let reserved = self.reserved_ports(); + if !reserved.is_empty() { + writeln!(f, "reserved port ranges:")?; + for ports in reserved.iter() { + writeln!(with_indent!(f), "{ports}")?; + } } writeln!(f, "allocated ports:")?; diff --git a/nat/src/masquerade/apalloc/mod.rs b/nat/src/masquerade/apalloc/mod.rs index 4d33677244..fbb52cd720 100644 --- a/nat/src/masquerade/apalloc/mod.rs +++ b/nat/src/masquerade/apalloc/mod.rs @@ -103,6 +103,7 @@ mod natip_with_bitmap; mod pool_fuzz; mod port_alloc; mod region; +mod reserved; mod setup; mod test_alloc; diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 0fd4d5cbc0..177291cbf0 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -458,13 +458,9 @@ impl ReservedConfig { /// through one expose still has to hold against an allocation made through another, or masquerade /// would hand out a port that port forwarding is statically mapping elsewhere. /// -/// # Ignored: this does not hold today -/// -/// A pool keeps at most one reserved port range per public address, so several claims on one -/// address collapse to whichever was recorded last and the rest are silently handed out. See -/// [`several_claims_on_one_address_are_all_honoured`] for the minimal case, and the note there for -/// the two places that need to change. Unignore both once they do. -#[ignore = "a pool holds one reserved port range per address; see several_claims_on_one_address_are_all_honoured"] +/// This is a property of the pools, which are given claims already expressed in public space. +/// Computing those claims from the configuration is a separate step, and gets the address space +/// wrong; see the note on `find_masquerade_portfw_overlap`. #[test] fn reserved_ports_are_never_allocated() { bolero::check!() @@ -498,23 +494,11 @@ fn reserved_ports_are_never_allocated() { } /// The minimal shape behind [`reserved_ports_are_never_allocated`]: one public address carrying -/// two port-forwarding claims. -/// -/// # Ignored: this does not hold today -/// -/// `build_reserved_prefixes_ports` records the claims in a `DisjointRangesBTreeMap` keyed by -/// address range, so two claims on one address are inserted under the same key and the second -/// replaces the first. Even with that fixed, `NatPool::use_new_ip` resolves a single -/// `Option` per address and `PortAllocator` stores one `reserved_port_range`, so the -/// data model cannot hold more than one claim per address either. Both need to take a set of -/// ranges. +/// two port-forwarding claims, both of which have to be honoured. /// -/// This is not a consequence of allocating from regions; the same collapse existed when each -/// expose had its own pool. It stays latent in production only because the claims are currently -/// computed from private prefixes and never match the public address they are looked up by, which -/// is the separate defect noted on `find_masquerade_portfw_overlap`. Fixing that without fixing -/// this would turn an inert path into a wrong one. -#[ignore = "a pool holds one reserved port range per address, so the earlier claim is dropped"] +/// This used to hold only the last claim recorded on an address, at three points in a row: the +/// claims were collected into a map keyed by address range, the pool resolved one range per +/// address out of it, and the port allocator stored one range. Each now carries the whole set. #[test] fn several_claims_on_one_address_are_all_honoured() { let address: u128 = BASE; diff --git a/nat/src/masquerade/apalloc/port_alloc.rs b/nat/src/masquerade/apalloc/port_alloc.rs index a5bf1cd4ab..e3d278f2b5 100644 --- a/nat/src/masquerade/apalloc/port_alloc.rs +++ b/nat/src/masquerade/apalloc/port_alloc.rs @@ -10,6 +10,7 @@ use super::NatIpWithBitmap; use super::alloc::AllocatedIp; +use super::reserved::PortClaims; use crate::masquerade::allocation::AllocatorError; use crate::port::NatPort; use concurrency::concurrency_mode; @@ -85,7 +86,7 @@ pub(crate) struct PortAllocator { current_alloc_index: AtomicUsize, thread_blocks: ThreadPortMap, allocated_blocks: AllocatedPortBlockMap, - reserved_port_range: Option, + reserved_ports: PortClaims, exclude_wellknown_ports: bool, } @@ -98,7 +99,7 @@ const IANA_WELLKNOWN_BLOCKS: u16 = IANA_WELLKNOWN_PORT_LIMIT / 256; impl PortAllocator { pub(crate) fn new( - reserved_port_range: Option, + reserved_ports: PortClaims, randomize: bool, exclude_wellknown_ports: bool, ) -> Self { @@ -133,17 +134,17 @@ impl PortAllocator { current_alloc_index: AtomicUsize::new(0), thread_blocks: ThreadPortMap::new(), allocated_blocks: AllocatedPortBlockMap::new(), - reserved_port_range, + reserved_ports, exclude_wellknown_ports, } } #[cfg(test)] pub(crate) fn new_no_randomness( - reserved_port_range: Option, + reserved_ports: PortClaims, exclude_wellknown_ports: bool, ) -> Self { - Self::new(reserved_port_range, false, exclude_wellknown_ports) + Self::new(reserved_ports, false, exclude_wellknown_ports) } #[concurrency_mode(std)] @@ -227,25 +228,11 @@ impl PortAllocator { return false; } - // Check if this block is fully contained in the reserved range - if let Some(reserved_range) = self.reserved_port_range - && reserved_range.len() >= 255 - { - // Corner case: reserved_range is 1-255+, but 0 cannot be allocated so - // reserved_range effectively renders the block unusable (except maybe for ICMP - // but never mind) - let adjusted_reserved_range = if reserved_range.start() == 1 { - PortRange::new(0, reserved_range.end()).unwrap_or_else(|_| unreachable!()) - } else { - reserved_range - }; - - let block_range = PortRange::from(*block); - if adjusted_reserved_range.covers(block_range) { - return false; - } - } - true + // Skip a block port forwarding has claimed in full: there is nothing left in it to + // hand out. Several claims may cover a block between them while no single one of + // them does, so this asks the claims as a whole rather than testing them one by + // one. + !self.reserved_ports.covers_block(block.to_port_number()) }) .ok_or(AllocatorError::NoPortBlock)?; Ok((index, block.to_port_number())) @@ -269,20 +256,12 @@ impl PortAllocator { self.usable_blocks .fetch_sub(1, concurrency::sync::atomic::Ordering::Relaxed); - let reserved_port_range_for_block = self.reserved_port_range.and_then(|range| { - range.intersection( - PortRange::new(base_port_index, base_port_index + 255) - .unwrap_or_else(|_| unreachable!()), - ) - }); + // Clip the claims to this block before they reach its bitmap, which indexes ports modulo + // 256 and cannot represent a range reaching past the block's end. + let reserved_for_block: Vec = + self.reserved_ports.within_block(base_port_index).collect(); - AllocatedPortBlock::new( - ip, - index, - base_port_index, - reserved_port_range_for_block, - allow_null, - ) + AllocatedPortBlock::new(ip, index, base_port_index, &reserved_for_block, allow_null) } pub(crate) fn allocate_port( @@ -341,11 +320,16 @@ impl PortAllocator { ) -> Result>, AllocatorError> { self.usable_blocks .fetch_sub(1, concurrency::sync::atomic::Ordering::Relaxed); + let base_port_index = (port.as_u16() / 256) * 256; // discard the offset within the block + // A block entering the allocator through a reservation serves later allocations like any + // other, so it carries the same claims, clipped to it. + let reserved_for_block: Vec = + self.reserved_ports.within_block(base_port_index).collect(); let block = Arc::new(AllocatedPortBlock::new( ip, index, - (port.as_u16() / 256) * 256, // port block base index, discard offset within block - None, + base_port_index, + &reserved_for_block, allow_null, )?); self.allocated_blocks @@ -391,8 +375,8 @@ impl PortAllocator { block.reserve_port_from_block(port) } - pub(crate) fn reserved_port_range(&self) -> Option { - self.reserved_port_range + pub(crate) fn reserved_ports(&self) -> &PortClaims { + &self.reserved_ports } // Used for Display @@ -427,7 +411,7 @@ impl AllocatedPortBlock { ip: Arc>, index: usize, base_port_idx: u16, - reserved_port_range: Option, + reserved_ports: &[PortRange], allow_null: bool, ) -> Result { let block = Self { @@ -438,8 +422,7 @@ impl AllocatedPortBlock { }; // Port 0 may be reserved, in which case we don't want to use it, so we mark it as not free. let reserve_zero = !allow_null && block.base_port_idx == 0; - let reserve_range = reserved_port_range.is_some(); - if reserve_zero || reserve_range { + if reserve_zero || !reserved_ports.is_empty() { let mut mutex_guard = block.usage_bitmap.lock(); if reserve_zero { mutex_guard.reserve_port_from_bitmap(0).map_err(|()| { @@ -448,12 +431,11 @@ impl AllocatedPortBlock { ) })?; } - if reserve_range { + // Reserving in the bitmap is an OR, so claims that overlap each other, or that + // overlap port 0 reserved just above, are harmless in any order. + for claim in reserved_ports { mutex_guard - .reserve_port_range_from_bitmap( - // We just check that reserved_port_range.is_some() - reserved_port_range.unwrap_or_else(|| unreachable!()), - ) + .reserve_port_range_from_bitmap(*claim) .map_err(|()| { AllocatorError::InternalIssue( "Failed to reserve port range from new block".to_string(), @@ -1257,7 +1239,7 @@ mod tests { #[test] fn pick_available_block_no_reserved_range() { - let allocator = PortAllocator::::new_no_randomness(None, false); + let allocator = PortAllocator::::new_no_randomness(PortClaims::default(), false); let (index, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(index, 0); assert_eq!(base_port, 0); @@ -1267,7 +1249,8 @@ mod tests { fn pick_available_block_reserved_range_covers_first_block() { // Reserve 0..=255 (entire first block) → should skip to block 1 (ports 256-511) let reserved = PortRange::new(0, 255).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); let (index, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(index, 1); assert_eq!(base_port, 256); @@ -1279,7 +1262,8 @@ mod tests { // be allocated anyway, so the block is effectively unusable. The code adjusts the // reserved range to start at 0, causing the block to be skipped. let reserved = PortRange::new(1, 255).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); let (index, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(index, 1); assert_eq!(base_port, 256); @@ -1289,7 +1273,8 @@ mod tests { fn pick_available_block_reserved_range_covers_multiple_blocks() { // Reserve 0..=511 (first two blocks) → should skip to block 2 (ports 512-767) let reserved = PortRange::new(0, 511).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); let (index, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(index, 2); assert_eq!(base_port, 512); @@ -1299,7 +1284,8 @@ mod tests { fn pick_available_block_reserved_range_does_not_cover_other_blocks() { // Reserve 0..=255 only covers block 0, block 1 is unaffected let reserved = PortRange::new(0, 255).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); // First pick skips block 0, gets block 1 let (_, base_port1) = allocator.pick_available_block().unwrap(); assert_eq!(base_port1, 256); @@ -1313,7 +1299,8 @@ mod tests { // Reserve 1..=200 (len 200 < 255) → block is NOT skipped entirely, individual ports // are reserved within the block instead let reserved = PortRange::new(1, 200).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); let (index, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(index, 0); assert_eq!(base_port, 0); @@ -1323,7 +1310,8 @@ mod tests { fn pick_available_block_reserved_middle_block() { // Reserve 256..=511 (block 1 only) → block 0 is fine, block 1 is skipped let reserved = PortRange::new(256, 511).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); // First pick: block 0 let (_, base_port1) = allocator.pick_available_block().unwrap(); assert_eq!(base_port1, 0); @@ -1336,7 +1324,8 @@ mod tests { fn pick_available_block_all_blocks_reserved() { // Reserve 0..=65535 (all blocks) → NoPortBlock error let reserved = PortRange::new(0, 65535).unwrap(); - let allocator = PortAllocator::::new_no_randomness(Some(reserved), false); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), false); assert!(allocator.pick_available_block().is_err()); } @@ -1467,7 +1456,7 @@ mod tests { fn exclude_wellknown_ports_first_available_block_is_1024() { // With no randomness and IANA exclusion, blocks 0-3 (ports 0-1023) are pre-marked // non-free, so the first block handed out should start at port 1024. - let allocator = PortAllocator::::new_no_randomness(None, true); + let allocator = PortAllocator::::new_no_randomness(PortClaims::default(), true); let (_, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(base_port, 1024); } @@ -1476,7 +1465,7 @@ mod tests { fn exclude_wellknown_ports_all_252_blocks_are_above_1023() { // Exactly 252 blocks (256 - 4 IANA blocks) should be allocatable; every one should // start at port >= 1024. The 253rd attempt should fail with NoPortBlock. - let allocator = PortAllocator::::new_no_randomness(None, true); + let allocator = PortAllocator::::new_no_randomness(PortClaims::default(), true); for _ in 0..252 { let (_, base_port) = allocator.pick_available_block().unwrap(); assert!( @@ -1490,7 +1479,7 @@ mod tests { #[test] fn exclude_wellknown_ports_disabled_starts_at_port_zero() { // Sanity check: without the flag, block 0 (port 0) is returned first. - let allocator = PortAllocator::::new_no_randomness(None, false); + let allocator = PortAllocator::::new_no_randomness(PortClaims::default(), false); let (_, base_port) = allocator.pick_available_block().unwrap(); assert_eq!(base_port, 0); } @@ -1498,7 +1487,8 @@ mod tests { #[test] fn exclude_wellknown_ports_combined_with_reserved_range() { let reserved = PortRange::new(2048, 2303).unwrap(); // entire block 8 - let allocator = PortAllocator::::new_no_randomness(Some(reserved), true); + let allocator = + PortAllocator::::new_no_randomness([reserved].into_iter().collect(), true); let (_, b0) = allocator.pick_available_block().unwrap(); assert_eq!(b0, 1024); // block 4 diff --git a/nat/src/masquerade/apalloc/reserved.rs b/nat/src/masquerade/apalloc/reserved.rs new file mode 100644 index 0000000000..941a99870a --- /dev/null +++ b/nat/src/masquerade/apalloc/reserved.rs @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Ports that port forwarding has claimed on public addresses, and that masquerade must not hand +//! out. +//! +//! A public `(address, port)` may only be live once towards a given peer, and port forwarding maps +//! its own statically, so masquerade allocating a pair port forwarding has taken would send return +//! traffic to whichever of the two the flow table happened to keep. +//! +//! One address can carry several claims. 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 +//! claims on an address are kept as a list and every one of them applies. Holding a single range +//! per address instead would silently honour whichever was recorded last. + +use crate::ranges::IpRange; +use lpm::prefix::PortRange; +use std::net::IpAddr; + +/// Ports claimed on public addresses, as a flat list of claims. +/// +/// Kept flat rather than keyed by address, because claims may be made on overlapping address +/// ranges and every claim covering an address has to be honoured, not just the innermost or the +/// last recorded. +#[derive(Debug, Clone, Default)] +pub(crate) struct ReservedPorts { + claims: Vec<(IpRange, PortRange)>, +} + +impl ReservedPorts { + pub(crate) fn is_empty(&self) -> bool { + self.claims.is_empty() + } + + pub(crate) fn claim(&mut self, addresses: IpRange, ports: PortRange) { + self.claims.push((addresses, ports)); + } + + /// Every port range claimed on `address`. + pub(crate) fn for_address(&self, address: IpAddr) -> PortClaims { + PortClaims( + self.claims + .iter() + .filter(|(addresses, _)| addresses.contains(&address)) + .map(|(_, ports)| *ports) + .collect(), + ) + } + + /// The claims, for display. + pub(crate) fn iter(&self) -> impl Iterator + '_ { + self.claims.iter().copied() + } +} + +/// The port ranges claimed on one public address. +/// +/// The ranges are independent and may overlap; nothing here merges them, because reserving a port +/// in a bitmap is idempotent and reserving the same port twice is harmless. +#[derive(Debug, Clone, Default)] +pub(crate) struct PortClaims(Vec); + +impl PortClaims { + pub(crate) fn is_empty(&self) -> bool { + self.0.is_empty() + } + + /// The claims, for display. + pub(crate) fn iter(&self) -> impl Iterator + '_ { + self.0.iter().copied() + } + + /// The claims that fall in the 256-port block starting at `base`, each clipped to it. + /// + /// Clipping here is what lets a claim span block boundaries: the bitmap of a block indexes + /// ports modulo 256 and cannot represent a range reaching past its end. + pub(crate) fn within_block(&self, base: u16) -> impl Iterator + '_ { + let block = block_range(base); + self.0 + .iter() + .filter_map(move |claim| claim.intersection(block)) + } + + /// Whether every port the block at `base` could hand out is claimed, so the block is of no use + /// to masquerade at all. + /// + /// Port 0 is never handed out for TCP or UDP, so a claim over the whole of a block bar port 0 + /// still leaves nothing allocatable. Several claims may cover a block between them while no + /// single one of them does, which is why this walks the union rather than testing each claim. + pub(crate) fn covers_block(&self, base: u16) -> bool { + let block = block_range(base); + // Port 0 is never handed out for TCP or UDP, so the first block only has to be covered + // from port 1 to be useless. + let must_cover_from = if base == 0 { 1 } else { base }; + + let mut claims: Vec = self.within_block(base).collect(); + claims.sort_by_key(PortRange::start); + + let mut covered_through: Option = None; + for claim in claims { + match covered_through { + // The block has to be covered from its first allocatable port. + None if claim.start() > must_cover_from => return false, + None => covered_through = Some(claim.end()), + // A claim starting more than one port past what is covered leaves a gap, and a + // port left in a gap is one masquerade may still hand out. + Some(end) if claim.start() > end.saturating_add(1) => return false, + Some(end) => covered_through = Some(end.max(claim.end())), + } + } + covered_through.is_some_and(|end| end >= block.end()) + } +} + +impl FromIterator for PortClaims { + fn from_iter>(iter: T) -> Self { + Self(iter.into_iter().collect()) + } +} + +fn block_range(base: u16) -> PortRange { + // A block is 256 ports wide and based at a multiple of 256, so this cannot overflow. + PortRange::new(base, base.saturating_add(255)).unwrap_or_else(|_| unreachable!()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn ports(start: u16, end: u16) -> PortRange { + PortRange::new(start, end).unwrap_or_else(|_| unreachable!()) + } + + fn claims(ranges: &[(u16, u16)]) -> PortClaims { + ranges.iter().map(|&(s, e)| ports(s, e)).collect() + } + + #[test] + fn no_claims_cover_nothing() { + assert!(!PortClaims::default().covers_block(1024)); + assert!(PortClaims::default().is_empty()); + } + + #[test] + fn a_claim_over_the_whole_block_covers_it() { + assert!(claims(&[(1024, 1279)]).covers_block(1024)); + } + + #[test] + fn a_claim_over_part_of_the_block_does_not() { + assert!(!claims(&[(1024, 1200)]).covers_block(1024)); + assert!(!claims(&[(1100, 1279)]).covers_block(1024)); + } + + // The case a per-claim test gets wrong: neither claim covers the block, but together they do. + #[test] + fn two_claims_covering_between_them_cover_the_block() { + assert!(claims(&[(1024, 1150), (1151, 1279)]).covers_block(1024)); + assert!(claims(&[(1024, 1150), (1150, 1279)]).covers_block(1024)); + } + + #[test] + fn two_claims_leaving_a_gap_do_not_cover_the_block() { + assert!(!claims(&[(1024, 1150), (1152, 1279)]).covers_block(1024)); + } + + // Port 0 is never allocatable, so covering 1..=255 leaves the first block useless. + #[test] + fn the_first_block_is_covered_without_port_zero() { + assert!(claims(&[(1, 255)]).covers_block(0)); + assert!(claims(&[(0, 255)]).covers_block(0)); + assert!(!claims(&[(1, 254)]).covers_block(0)); + } + + #[test] + fn claims_are_clipped_to_the_block() { + // A claim spanning two blocks reaches each of them, clipped. + let spanning = claims(&[(1000, 1300)]); + let in_first: Vec<_> = spanning.within_block(1024).collect(); + assert_eq!(in_first, vec![ports(1024, 1279)]); + let in_second: Vec<_> = spanning.within_block(1280).collect(); + assert_eq!(in_second, vec![ports(1280, 1300)]); + // And not blocks it does not touch. + assert_eq!(spanning.within_block(2048).count(), 0); + } + + #[test] + fn claims_outside_the_block_are_ignored() { + assert!(!claims(&[(2048, 2303)]).covers_block(1024)); + } +} diff --git a/nat/src/masquerade/apalloc/setup.rs b/nat/src/masquerade/apalloc/setup.rs index f5b3a32ca0..6d8eb698b2 100644 --- a/nat/src/masquerade/apalloc/setup.rs +++ b/nat/src/masquerade/apalloc/setup.rs @@ -11,13 +11,12 @@ use super::alloc::{IpAllocator, NatPool, PoolSet}; use super::region::{AddrInterval, Region, decompose, regions_by_owner}; +use super::reserved::ReservedPorts; use super::{NatAllocator, NatIpWithBitmap, PoolTable, PoolTableKey}; use crate::masquerade::allocator_writer::MasqueradeConfig; use crate::masquerade::natip::NatIp; -use crate::ranges::IpRange; use config::external::overlay::vpcpeering::{ValidatedExpose, ValidatedManifest}; -use lpm::prefix::range_map::DisjointRangesBTreeMap; -use lpm::prefix::{L4Protocol, PortRange, PrefixPortsSet, PrefixWithOptionalPorts}; +use lpm::prefix::{L4Protocol, PrefixPortsSet, PrefixWithOptionalPorts}; use net::ip::NextHeader; use net::packet::VpcDiscriminant; use std::collections::BTreeMap; @@ -274,7 +273,7 @@ fn build_region_allocators( let pool = NatPool::for_range( region.range, - build_reserved_prefixes_ports(&reserved), + build_reserved_ports(&reserved), exclude_wellknown_ports, ); IpAllocator::new(pool, randomize) @@ -310,22 +309,21 @@ fn find_masquerade_portfw_overlap<'a>( reserve_sets } -fn build_reserved_prefixes_ports( +// Every claim is kept, including several on one address: a set of claims is not a map from +// address to port range, and recording it as one silently honoured whichever came last. +fn build_reserved_ports( prefixes_and_ports_to_exclude_from_pools: &PrefixPortsSet, -) -> Option> { - if prefixes_and_ports_to_exclude_from_pools.is_empty() { - return None; - } - let mut reserved_prefixes_ports = DisjointRangesBTreeMap::new(); +) -> ReservedPorts { + let mut reserved = ReservedPorts::default(); for prefix in prefixes_and_ports_to_exclude_from_pools { debug_assert!(prefix.ports().is_some()); let Some(ports) = prefix.ports() else { error!("Stepped on a port-forwarding prefix without ports. This is a bug"); continue; }; - reserved_prefixes_ports.insert(prefix.prefix().into(), ports); + reserved.claim(prefix.prefix().into(), ports); } - Some(reserved_prefixes_ports) + reserved } fn pool_table_key_for_expose( diff --git a/nat/src/ranges.rs b/nat/src/ranges.rs index 14b3d287b7..ed5b89dab7 100644 --- a/nat/src/ranges.rs +++ b/nat/src/ranges.rs @@ -138,7 +138,6 @@ impl IpRange { self.end } - #[cfg(test)] #[must_use] pub fn contains(&self, addr: &IpAddr) -> bool { self.start <= *addr && *addr <= self.end From bfa935ee9b891af06fd46941cef67f045cd59c0d Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 18:42:49 -0600 Subject: [PATCH 12/24] fix(masquerade): Claim port-forwarded ports in the public space 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 9 +- nat/src/masquerade/apalloc/pool_fuzz.rs | 42 ++- nat/src/masquerade/apalloc/setup.rs | 311 +++++++----------- nat/src/masquerade/apalloc/test_alloc.rs | 99 ++++++ 4 files changed, 251 insertions(+), 210 deletions(-) diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs index 2692a68ed5..d199146c35 100644 --- a/nat/src/masquerade/apalloc/concurrent_fuzz.rs +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -184,7 +184,8 @@ impl Published { generation: u64, survivors: &[(usize, Ipv4Addr, NatPort)], ) -> Self { - let pools = pool_sets_for_specs::(specs, NextHeader::TCP, false); + let pools = + pool_sets_for_specs::(specs, &PrefixPortsSet::new(), NextHeader::TCP, false); let mut carried = BTreeSet::new(); let mut reservations = Vec::new(); @@ -277,7 +278,6 @@ impl Scenario { AddrInterval::new(BASE + start, BASE + end) }) .collect(), - reserved: PrefixPortsSet::new(), idle_timeout: IDLE_TIMEOUT, }) .collect() @@ -289,7 +289,8 @@ impl Scenario { let specs = self.specs(); // The flows that already exist when the config change arrives. - let initial = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let initial = + pool_sets_for_specs::(&specs, &PrefixPortsSet::new(), NextHeader::TCP, false); let mut existing = Vec::new(); let mut survivors = Vec::new(); for (owner, pool) in initial.iter().enumerate() { @@ -470,11 +471,11 @@ fn printing_the_pool_does_not_wedge_it_against_a_flow_ending() { let specs = vec![PoolSpec { // One address, so the flow that ends is the last holder of the one being printed. public_ranges: vec![AddrInterval::new(BASE, BASE)], - reserved: PrefixPortsSet::new(), idle_timeout: IDLE_TIMEOUT, }]; let pools = Arc::new(pool_sets_for_specs::( &specs, + &PrefixPortsSet::new(), NextHeader::TCP, false, )); diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 177291cbf0..7a51a5e39e 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -88,7 +88,6 @@ impl Config { .into_iter() .map(|public_ranges| PoolSpec { public_ranges, - reserved: PrefixPortsSet::new(), idle_timeout: IDLE_TIMEOUT, }) .collect() @@ -96,7 +95,12 @@ impl Config { fn pool_sets(&self) -> Vec> { // No randomization: a failure has to reproduce from its seed alone. - pool_sets_for_specs::(&self.specs(), NextHeader::TCP, false) + pool_sets_for_specs::( + &self.specs(), + &PrefixPortsSet::new(), + NextHeader::TCP, + false, + ) } fn owner_count(&self) -> usize { @@ -278,11 +282,11 @@ fn an_exhausted_region_falls_through_to_the_next() { let specs = vec![PoolSpec { public_ranges: vec![AddrInterval::new(full, full), AddrInterval::new(free, free)], - reserved: [every_port].into_iter().collect(), idle_timeout: IDLE_TIMEOUT, }]; - let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let claimed = PrefixPortsSet::from([every_port]); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); let allocation = pool_sets[0] .allocate(false) .expect("the second region has room, so allocation must succeed"); @@ -310,10 +314,10 @@ fn an_address_past_the_indexable_span_is_refused_rather_than_panicking() { // Far wider than the bitmap can index. let specs = vec![PoolSpec { public_ranges: vec![AddrInterval::new(start, start + (1u128 << 40))], - reserved: PrefixPortsSet::new(), idle_timeout: IDLE_TIMEOUT, }]; - let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let pool_sets = + pool_sets_for_specs::(&specs, &PrefixPortsSet::new(), NextHeader::TCP, false); let port = NatPort::new_port_checked(4096).unwrap_or_else(|_| unreachable!()); // Just inside the indexable span: an ordinary carry-over, which must still work. @@ -340,10 +344,10 @@ fn ipv6_pools_allocate_within_their_range() { let end = start + 3; let specs = vec![PoolSpec { public_ranges: vec![AddrInterval::new(start, end)], - reserved: PrefixPortsSet::new(), idle_timeout: IDLE_TIMEOUT, }]; - let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let pool_sets = + pool_sets_for_specs::(&specs, &PrefixPortsSet::new(), NextHeader::TCP, false); let mut held = Vec::new(); let mut seen = BTreeSet::new(); @@ -440,14 +444,20 @@ impl ReservedConfig { .config .owner_ranges() .into_iter() - .zip(&self.reservations) - .map(|(public_ranges, claims)| PoolSpec { + .map(|public_ranges| PoolSpec { public_ranges, - reserved: claims.iter().map(|claim| claim.as_prefix()).collect(), idle_timeout: IDLE_TIMEOUT, }) .collect(); - pool_sets_for_specs::(&specs, NextHeader::TCP, false) + // A claim binds the public space towards a peer, whichever expose declared it, so the + // claims of every expose apply to every pool built over that space. + let claimed: PrefixPortsSet = self + .reservations + .iter() + .flatten() + .map(|claim| claim.as_prefix()) + .collect(); + pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false) } } @@ -459,8 +469,8 @@ impl ReservedConfig { /// would hand out a port that port forwarding is statically mapping elsewhere. /// /// This is a property of the pools, which are given claims already expressed in public space. -/// Computing those claims from the configuration is a separate step, and gets the address space -/// wrong; see the note on `find_masquerade_portfw_overlap`. +/// Translating a configuration into those claims is a separate step, covered by the tests on +/// `ReserveSets` in `setup.rs` and end to end by `test_forwarded_ports_are_not_masqueraded_onto`. #[test] fn reserved_ports_are_never_allocated() { bolero::check!() @@ -511,11 +521,11 @@ fn several_claims_on_one_address_are_all_honoured() { let specs = vec![PoolSpec { public_ranges: vec![AddrInterval::new(address, address)], - reserved: [claim(1024, 1024), claim(2000, 2000)].into_iter().collect(), idle_timeout: IDLE_TIMEOUT, }]; - let pool_sets = pool_sets_for_specs::(&specs, NextHeader::TCP, false); + let claimed = PrefixPortsSet::from([claim(1024, 1024), claim(2000, 2000)]); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); let allocated: BTreeSet = allocate_round_robin(&pool_sets, 4) .iter() .map(|(_, allocation)| allocation.port().as_u16()) diff --git a/nat/src/masquerade/apalloc/setup.rs b/nat/src/masquerade/apalloc/setup.rs index 6d8eb698b2..4e8b347a13 100644 --- a/nat/src/masquerade/apalloc/setup.rs +++ b/nat/src/masquerade/apalloc/setup.rs @@ -59,10 +59,17 @@ struct GatheredExpose<'a> { // The public range this expose allocates from, as raw address intervals. public_ranges: Vec, idle_timeout: Duration, - reserved: ReserveSets, } -/// Ports that port forwarding has claimed, and that masquerade must not hand out. +/// Everything masquerading towards one peer VPC: the exposes that allocate from its public space, +/// and the public ports port forwarding has already claimed in that same space. +#[derive(Default)] +struct GatheredGroup<'a> { + exposes: Vec>, + claimed: ReserveSets, +} + +/// Public ports that port forwarding has claimed, and that masquerade must not hand out. #[derive(Debug, Default, Clone, PartialEq, Eq)] struct ReserveSets { tcp: PrefixPortsSet, @@ -78,6 +85,22 @@ impl ReserveSets { _ => None, } } + + // Record what a port-forwarding expose has taken, under the protocols it applies to. The claim + // is on the public side, which is the space masquerade allocates from and the space a pool is + // asked about; the private side describes addresses the pools never see. + fn add(&mut self, expose: &ValidatedExpose) { + let claimed = expose.as_range_or_empty(); + let proto = expose.nat().map_or(L4Protocol::Any, |nat| nat.proto); + match proto { + L4Protocol::Tcp => self.tcp.extend(claimed.clone()), + L4Protocol::Udp => self.udp.extend(claimed.clone()), + L4Protocol::Any => { + self.tcp.extend(claimed.clone()); + self.udp.extend(claimed.clone()); + } + } + } } // Collect the masquerade exposes of every peering, grouped by the VPC they masquerade towards. @@ -87,7 +110,7 @@ fn gather_exposes<'a, J, F, FIter, P, PIter>( config: &'a MasqueradeConfig, exposes_filter: &F, port_forwarding_exposes_filter: &P, -) -> BTreeMap>> +) -> BTreeMap> where J: NatIp, F: Fn(&'a ValidatedManifest) -> FIter, @@ -95,12 +118,20 @@ where P: Fn(&'a ValidatedManifest) -> PIter, PIter: Iterator, { - let mut groups: BTreeMap>> = BTreeMap::new(); + let mut groups: BTreeMap> = BTreeMap::new(); for nat_peering in config.iter() { let manifest = nat_peering.peering.local(); - let port_forwarding_exposes: Vec<&'a ValidatedExpose> = - port_forwarding_exposes_filter(manifest).collect(); + let group = groups.entry(nat_peering.dst_vpcd).or_default(); + + // Port forwarding claims public space towards this peer whichever VPC declared it, because + // the public space towards a peer is shared: return traffic carries nothing that says + // which VPC it belongs to. Claims are therefore collected per peer VPC rather than per + // manifest, so a claim made by one VPC is honoured by another VPC's pools, and a peering + // that port-forwards without masquerading still has its claims respected. + for pf_expose in port_forwarding_exposes_filter(manifest) { + group.claimed.add(pf_expose); + } for expose in exposes_filter(manifest) { let public_ranges = public_intervals::(expose.as_range_or_empty()); @@ -109,18 +140,14 @@ where // happens if none of its prefixes are of the version we are building. continue; } - groups - .entry(nat_peering.dst_vpcd) - .or_default() - .push(GatheredExpose { - src_vpc_id: nat_peering.src_vpcd, - private_prefixes: expose.ips(), - public_ranges, - idle_timeout: expose - .idle_timeout() - .unwrap_or(DEFAULT_MASQUERADE_IDLE_TIMEOUT), - reserved: find_masquerade_portfw_overlap(&port_forwarding_exposes, expose), - }); + group.exposes.push(GatheredExpose { + src_vpc_id: nat_peering.src_vpcd, + private_prefixes: expose.ips(), + public_ranges, + idle_timeout: expose + .idle_timeout() + .unwrap_or(DEFAULT_MASQUERADE_IDLE_TIMEOUT), + }); } } @@ -164,26 +191,27 @@ fn build_pools_generic<'a, I, J, F, FIter, P, PIter>( let groups = gather_exposes::(config, &exposes_filter, &port_forwarding_exposes_filter); - for (dst_vpc_id, exposes) in groups { + for (dst_vpc_id, group) in groups { // Allocations for TCP, for example, do not affect allocations for UDP or for ICMP: the // space made of addresses and L4 ports or identifiers is distinct for each protocol. So // each region backs one allocator per protocol, over the same addresses. for protocol in [NextHeader::TCP, NextHeader::UDP, icmp_proto] { - let specs: Vec = exposes + let specs: Vec = group + .exposes .iter() .map(|expose| PoolSpec { public_ranges: expose.public_ranges.clone(), - reserved: expose - .reserved - .for_protocol(protocol) - .cloned() - .unwrap_or_default(), idle_timeout: expose.idle_timeout, }) .collect(); - let pool_sets = pool_sets_for_specs::(&specs, protocol, randomize); - for (expose, pool_set) in exposes.iter().zip(pool_sets) { + let claimed = group + .claimed + .for_protocol(protocol) + .cloned() + .unwrap_or_default(); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, protocol, randomize); + for (expose, pool_set) in group.exposes.iter().zip(pool_sets) { add_pool_entries( table, expose.private_prefixes, @@ -202,7 +230,6 @@ fn build_pools_generic<'a, I, J, F, FIter, P, PIter>( #[derive(Clone)] pub(crate) struct PoolSpec { pub(crate) public_ranges: Vec, - pub(crate) reserved: PrefixPortsSet, pub(crate) idle_timeout: Duration, } @@ -214,6 +241,7 @@ pub(crate) struct PoolSpec { /// ranges cover. pub(crate) fn pool_sets_for_specs( specs: &[PoolSpec], + claimed: &PrefixPortsSet, protocol: NextHeader, randomize: bool, ) -> Vec> { @@ -228,7 +256,7 @@ pub(crate) fn pool_sets_for_specs( specs.len() ); - let allocators = build_region_allocators::(®ions, specs, protocol, randomize); + let allocators = build_region_allocators::(®ions, claimed, protocol, randomize); let by_owner = regions_by_owner(®ions); specs @@ -251,7 +279,7 @@ pub(crate) fn pool_sets_for_specs( // keeps a public address and port from being handed out twice. fn build_region_allocators( regions: &[Region], - specs: &[PoolSpec], + claimed: &PrefixPortsSet, protocol: NextHeader, randomize: bool, ) -> Vec> { @@ -259,56 +287,21 @@ fn build_region_allocators( // ICMP identifiers are allocated independently and are not subject to that policy. let exclude_wellknown_ports = matches!(protocol, NextHeader::TCP | NextHeader::UDP); + // The claims cover the whole public space towards this peer VPC, so every region gets the + // same set and each pool keeps only what covers an address when it hands one out. A region is + // shared between exposes, and a claim binds the space rather than the expose that declared it, + // so there is nothing per-owner to work out here. + let reserved = build_reserved_ports(claimed); + regions .iter() .map(|region| { - // A region is shared, so it must honour every claim on it: reserve what port - // forwarding has taken from any of its owners. - let reserved = region - .owners - .iter() - .fold(PrefixPortsSet::new(), |accumulated, &owner| { - accumulated.union_prefixes_and_ports(&specs[owner].reserved) - }); - - let pool = NatPool::for_range( - region.range, - build_reserved_ports(&reserved), - exclude_wellknown_ports, - ); + let pool = NatPool::for_range(region.range, reserved.clone(), exclude_wellknown_ports); IpAllocator::new(pool, randomize) }) .collect() } -fn find_masquerade_portfw_overlap<'a>( - port_forwarding_exposes: &Vec<&'a ValidatedExpose>, - expose: &'a ValidatedExpose, -) -> ReserveSets { - let expose_nat = expose.nat().unwrap_or_else(|| unreachable!()); - let mut reserve_sets = ReserveSets::default(); - - for pf_expose in port_forwarding_exposes { - let pf_nat = pf_expose.nat().unwrap_or_else(|| unreachable!()); - let Some(relevant_proto) = expose_nat.proto.intersection(&pf_nat.proto) else { - // No overlap on L4 protocols, so no overlap for prefixes and ports. - continue; - }; - let ranges_intersection = pf_expose - .ips() - .intersection_prefixes_and_ports(expose.ips()); - match relevant_proto { - L4Protocol::Tcp => reserve_sets.tcp.extend(ranges_intersection), - L4Protocol::Udp => reserve_sets.udp.extend(ranges_intersection), - L4Protocol::Any => { - reserve_sets.tcp.extend(ranges_intersection.clone()); - reserve_sets.udp.extend(ranges_intersection); - } - } - } - reserve_sets -} - // Every claim is kept, including several on one address: a set of claims is not a map from // address to port range, and recording it as one silently honoured whichever came last. fn build_reserved_ports( @@ -360,7 +353,7 @@ fn prefix_bounds(prefix: &PrefixWithOptionalPorts) -> (I, I) { #[cfg(test)] mod tests { - use super::{ReserveSets, find_masquerade_portfw_overlap}; + use super::ReserveSets; use config::external::overlay::vpcpeering::VpcExpose; use lpm::prefix::{L4Protocol, PortRange, PrefixPortsSet, PrefixWithOptionalPorts}; @@ -368,145 +361,83 @@ mod tests { PrefixWithOptionalPorts::new(s.into(), Some(PortRange::new(start, end).unwrap())) } - // tests for find_masquerade_portfw_overlap() - - #[test] - fn find_masquerade_portfw_overlap_multiple_pf_exposes() { - let expose = VpcExpose::empty() - .make_masquerade(None) - .unwrap() - .ip("10.0.0.0/16".into()) - .ip("172.16.0.0/16".into()) - .as_range("192.168.0.0/16".into()) - .unwrap() - .validate() - .unwrap(); - let pf_expose1 = VpcExpose::empty() - .make_port_forwarding(None, None) - .unwrap() - .ip(prefix_with_ports("10.0.1.0/24", 8080, 8090)) - .as_range(prefix_with_ports("192.168.1.0/24", 8080, 8090)) - .unwrap() - .validate() - .unwrap(); - let pf_expose2 = VpcExpose::empty() - .make_port_forwarding(None, None) + // A port-forwarding expose, mapping a private range onto a public one. + fn port_forwarding(private: &str, public: &str, proto: Option) -> VpcExpose { + VpcExpose::empty() + .make_port_forwarding(None, proto) .unwrap() - .ip(prefix_with_ports("172.16.5.0/24", 8080, 8090)) - .as_range(prefix_with_ports("192.168.2.0/24", 8080, 8090)) + .ip(prefix_with_ports(private, 8080, 8090)) + .as_range(prefix_with_ports(public, 8080, 8090)) .unwrap() - .validate() - .unwrap(); - let pf_exposes_vec = vec![&pf_expose1, &pf_expose2]; - let result = find_masquerade_portfw_overlap(&pf_exposes_vec, &expose); - assert_eq!( - result, - ReserveSets { - tcp: PrefixPortsSet::from([ - prefix_with_ports("10.0.1.0/24", 8080, 8090), - prefix_with_ports("172.16.5.0/24", 8080, 8090), - ]), - udp: PrefixPortsSet::from([ - prefix_with_ports("10.0.1.0/24", 8080, 8090), - prefix_with_ports("172.16.5.0/24", 8080, 8090), - ]), - } - ); } + fn claims_of(exposes: &[VpcExpose]) -> ReserveSets { + let mut sets = ReserveSets::default(); + for expose in exposes { + sets.add(&expose.clone().validate().unwrap()); + } + sets + } + + // The claim is on the public address and ports, because that is what masquerade allocates and + // what a pool is asked about. Recording the private side instead described a space the pools + // never look in, so nothing was ever actually reserved. #[test] - fn find_masquerade_portfw_overlap_with_ports() { - let expose = VpcExpose::empty() - .make_masquerade(None) - .unwrap() - .ip("10.0.0.0/24".into()) - .as_range("192.168.0.0/24".into()) - .unwrap() - .validate() - .unwrap(); - let pf_expose = VpcExpose::empty() - .make_port_forwarding(None, None) - .unwrap() - .ip(prefix_with_ports("10.0.0.0/24", 8080, 8090)) - .as_range(prefix_with_ports("192.168.1.0/24", 8080, 8090)) - .unwrap() - .validate() - .unwrap(); - let pf_exposes_vec = vec![&pf_expose]; - let result = find_masquerade_portfw_overlap(&pf_exposes_vec, &expose); + fn a_claim_is_recorded_on_the_public_range() { + let claims = claims_of(&[port_forwarding("10.0.0.0/24", "192.168.1.0/24", None)]); + let expected = PrefixPortsSet::from([prefix_with_ports("192.168.1.0/24", 8080, 8090)]); assert_eq!( - result, + claims, ReserveSets { - tcp: PrefixPortsSet::from([prefix_with_ports("10.0.0.0/24", 8080, 8090)]), - udp: PrefixPortsSet::from([prefix_with_ports("10.0.0.0/24", 8080, 8090)]), + tcp: expected.clone(), + udp: expected, } ); } #[test] - fn find_masquerade_portfw_overlap_with_ports_tcp() { - let expose = VpcExpose::empty() - .make_masquerade(None) - .unwrap() - .ip("10.0.0.0/24".into()) - .as_range("192.168.0.0/24".into()) - .unwrap() - .validate() - .unwrap(); - let pf_expose = VpcExpose::empty() - .make_port_forwarding(None, Some(L4Protocol::Tcp)) // TCP only - .unwrap() - .ip(prefix_with_ports("10.0.0.0/24", 8080, 8090)) - .as_range(prefix_with_ports("192.168.1.0/24", 8080, 8090)) - .unwrap() - .validate() - .unwrap(); - let pf_exposes_vec = vec![&pf_expose]; - let result = find_masquerade_portfw_overlap(&pf_exposes_vec, &expose); + fn a_protocol_specific_claim_only_binds_that_protocol() { + let claims = claims_of(&[port_forwarding( + "10.0.0.0/24", + "192.168.1.0/24", + Some(L4Protocol::Tcp), + )]); assert_eq!( - result, + claims, ReserveSets { - tcp: PrefixPortsSet::from([prefix_with_ports("10.0.0.0/24", 8080, 8090)]), - udp: PrefixPortsSet::default() + tcp: PrefixPortsSet::from([prefix_with_ports("192.168.1.0/24", 8080, 8090)]), + udp: PrefixPortsSet::default(), } ); } #[test] - fn find_masquerade_portfw_overlap_duplicates_collapsed() { - // Two port-forwarding exposes with the same prefix should produce one entry - let expose = VpcExpose::empty() - .make_masquerade(None) - .unwrap() - .ip("10.0.0.0/16".into()) - .as_range("192.168.0.0/24".into()) - .unwrap() - .validate() - .unwrap(); - let pf_expose1 = VpcExpose::empty() - .make_port_forwarding(None, None) - .unwrap() - .ip(prefix_with_ports("10.0.1.0/24", 8080, 8090)) - .as_range(prefix_with_ports("192.168.1.0/24", 8080, 8090)) - .unwrap() - .validate() - .unwrap(); - let pf_expose2 = VpcExpose::empty() - .make_port_forwarding(None, None) - .unwrap() - .ip(prefix_with_ports("10.0.1.0/24", 8080, 8090)) - .as_range(prefix_with_ports("192.168.1.0/24", 8080, 8090)) - .unwrap() - .validate() - .unwrap(); - let pf_exposes_vec = vec![&pf_expose1, &pf_expose2]; - let result = find_masquerade_portfw_overlap(&pf_exposes_vec, &expose); + fn claims_from_several_exposes_accumulate() { + let claims = claims_of(&[ + port_forwarding("10.0.1.0/24", "192.168.1.0/24", None), + port_forwarding("172.16.5.0/24", "192.168.2.0/24", None), + ]); + let expected = PrefixPortsSet::from([ + prefix_with_ports("192.168.1.0/24", 8080, 8090), + prefix_with_ports("192.168.2.0/24", 8080, 8090), + ]); assert_eq!( - result, + claims, ReserveSets { - tcp: PrefixPortsSet::from([prefix_with_ports("10.0.1.0/24", 8080, 8090)]), - udp: PrefixPortsSet::from([prefix_with_ports("10.0.1.0/24", 8080, 8090)]), + tcp: expected.clone(), + udp: expected, } ); } + + // Two exposes forwarding different private ranges onto one public range describe one claim. + #[test] + fn duplicate_public_claims_collapse() { + let claims = claims_of(&[ + port_forwarding("10.0.1.0/24", "192.168.1.0/24", None), + port_forwarding("10.0.2.0/24", "192.168.1.0/24", None), + ]); + assert_eq!(claims.tcp.len(), 1); + assert_eq!(claims.udp.len(), 1); + } } diff --git a/nat/src/masquerade/apalloc/test_alloc.rs b/nat/src/masquerade/apalloc/test_alloc.rs index 41902accf2..d0afb50cdb 100644 --- a/nat/src/masquerade/apalloc/test_alloc.rs +++ b/nat/src/masquerade/apalloc/test_alloc.rs @@ -13,6 +13,7 @@ mod context { use crate::masquerade::apalloc::{NatAllocator, PoolTable, PoolTableKey}; use config::external::overlay::vpc::{Peering, ValidatedVpcTable, Vpc, VpcTable}; use config::external::overlay::vpcpeering::{VpcExpose, VpcManifest}; + use lpm::prefix::{PortRange, PrefixWithOptionalPorts}; use net::ip::NextHeader; use net::packet::VpcDiscriminant; use net::udp::UdpPort; @@ -338,6 +339,72 @@ mod context { NatAllocator::new(config) } + // VPC-1 masquerades onto a single public address towards VPC-3, while VPC-2 port-forwards a + // range of ports on that same public address towards the same VPC-3. The public space towards + // a peer is shared, so those ports are spoken for and masquerade may not hand them out: + // return traffic to one of them carries nothing that says which of the two it belongs to. + #[allow(dead_code)] + fn build_context_masquerade_over_forwarded_ports() -> ValidatedVpcTable { + let masquerade = VpcExpose::empty() + .make_masquerade(None) + .unwrap() + .ip("1.1.0.0/16".into()) + .as_range("10.1.0.0/32".into()) + .unwrap(); + // The forwarded ports sit on the very address VPC-1 masquerades onto. + let forwarded = VpcExpose::empty() + .make_port_forwarding(None, None) + .unwrap() + .ip(PrefixWithOptionalPorts::new( + "2.1.0.0/32".into(), + Some(PortRange::new(1024, 1030).unwrap()), + )) + .as_range(PrefixWithOptionalPorts::new( + "10.1.0.0/32".into(), + Some(PortRange::new(1024, 1030).unwrap()), + )) + .unwrap(); + let remote = + VpcManifest::with_exposes("VPC-3", vec![VpcExpose::empty().ip("3.0.0.0/24".into())]); + + let mut vpc1 = Vpc::new("VPC-1", "67890", vni1().as_u32()).unwrap(); + let mut vpc2 = Vpc::new("VPC-2", "12345", vni2().as_u32()).unwrap(); + let vpc3 = Vpc::new("VPC-3", "11111", vni3().as_u32()).unwrap(); + + vpc1.peerings.push(Peering { + name: "masquerading_peering".into(), + local: VpcManifest::with_exposes("VPC-1", vec![masquerade]), + remote: remote.clone(), + remote_id: "11111".try_into().unwrap(), + remote_vni: vpc3.vni, + gwgroup: "default".into(), + acl: None, + }); + vpc2.peerings.push(Peering { + name: "forwarding_peering".into(), + local: VpcManifest::with_exposes("VPC-2", vec![forwarded]), + remote, + remote_id: "11111".try_into().unwrap(), + remote_vni: vpc3.vni, + gwgroup: "default".into(), + acl: None, + }); + + let mut vpctable = VpcTable::new(); + vpctable.add(vpc1).unwrap(); + vpctable.add(vpc2).unwrap(); + vpctable.add(vpc3).unwrap(); + + vpctable.validate().unwrap() + } + + #[allow(dead_code)] + pub fn build_allocator_masquerade_over_forwarded_ports() -> NatAllocator { + let vpc_table = build_context_masquerade_over_forwarded_ports(); + let config = MasqueradeConfig::new(&vpc_table, 1).set_randomize(false); + NatAllocator::new(config) + } + #[allow(dead_code)] pub fn get_pool_set_v4( pool: &PoolTable, @@ -816,6 +883,38 @@ mod std_tests { } } + // Ports that port forwarding has claimed on a public address are not handed out by + // masquerade, even though the two were configured by different VPCs. The claim binds the + // public space towards the peer, not the expose that declared it. + // + // The claim is on the public side. It used to be computed from the private prefixes instead, + // which described a space the pools are never asked about, so nothing was reserved at all. + #[test] + fn test_forwarded_ports_are_not_masqueraded_onto() { + let allocator = build_allocator_masquerade_over_forwarded_ports(); + + let mut held = Vec::new(); + let mut ports = Vec::new(); + for _ in 0..32 { + let allocation = allocator + .allocate_v4(vpcd1(), vpcd3(), addr_v4("1.1.0.1"), NextHeader::TCP) + .unwrap(); + let ip = allocation.allocation.ip(); + let port = allocation.allocation.port().as_u16(); + assert_eq!(ip, addr_v4("10.1.0.0")); + assert!( + !(1024..=1030).contains(&port), + "masquerade handed out {ip}:{port}, which port forwarding has claimed" + ); + ports.push(port); + held.push(allocation); + } + + // Allocation starts at the lowest port it may use, so the first one lands just past the + // claim. Without the reservation it would be 1024, which is what makes this test bite. + assert_eq!(ports[0], 1031); + } + // Both VPCs keep their own entry, rather than the second overwriting the first. #[test] fn test_overlapping_private_prefixes_keep_separate_entries() { From fb73cfab0139ea2d51c7adc38d21c9a82bade922 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 19:37:22 -0600 Subject: [PATCH 13/24] fix(masquerade): Pass over an address that has no port to give 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 58 ++++++++++- nat/src/masquerade/apalloc/pool_fuzz.rs | 124 ++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 2 deletions(-) diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 9d983523ad..fbfd1250d3 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -24,6 +24,14 @@ use std::net::{IpAddr, Ipv6Addr}; use std::time::Duration; use tracing::{debug, error}; +/// How many addresses one allocation may draw before giving up. +/// +/// A data plane cannot search without a bound on the packet path. The bound costs nothing in the +/// ordinary case, where the first address serves, and an address that cannot serve is taken out of +/// the pool as it is found, so a run of them is worked through over successive packets rather than +/// being walked again each time. +const MAX_ADDRESSES_PER_ALLOCATION: usize = 8; + /////////////////////////////////////////////////////////////////////////////// // IpAllocator /////////////////////////////////////////////////////////////////////////////// @@ -109,12 +117,48 @@ impl IpAllocator { Ok(arc_ip) } + /// Take an address out of the pool for good. + /// + /// Only for an address that can never serve, not one that is merely busy. + fn retire_ip(&self, ip: I) { + self.pool.write().retire_from_pool(ip); + } + fn allocate_from_new_ip( &self, allow_null: bool, ) -> Result, AllocatorError> { - self.allocate_new_ip_from_pool() - .and_then(|ip| ip.allocate_port_for_ip(allow_null)) + let mut exhausted = None; + for _ in 0..MAX_ADDRESSES_PER_ALLOCATION { + let ip = self.allocate_new_ip_from_pool()?; + let address = ip.ip(); + match ip.allocate_port_for_ip(allow_null) { + Ok(port) => return Ok(port), + Err(e) if e.is_exhaustion() => { + // The address came fresh out of the pool, so what leaves it with no port is + // almost always fixed for the life of the pool: every port it has is spoken + // for by port forwarding, or by the well-known range. Take it out instead of + // handing it back, or the next allocation stops on it again and the pool + // serves nothing for as long as the address is the lowest one free. + // + // Almost always, because the address joins the in-use list before this runs, + // so another thread could in principle take its every port in between. Losing + // an address that way costs capacity rather than correctness, and needs 64k + // allocations to land in the window. + // + // The allocation attempt consumed the only strong reference we held, so the + // address is already back in the pool by now and taking it out is what sticks. + debug!("Address {address} has no port to give and is taken out of the pool"); + self.retire_ip(address); + exhausted = Some(e); + } + Err(e) => return Err(e), + } + } + // Addresses are only ever tried once, since a useless one is taken out as it is found, so + // stopping here spreads the discovery of a large claimed range over several packets rather + // than doing all of it on one. + Err(exhausted.unwrap_or(AllocatorError::NoFreeIp)) } fn cleanup_used_ips(&self) { @@ -458,6 +502,16 @@ impl NatPool { )) } + // Mark an address used and never give it back, for an address that can never serve. + fn retire_from_pool(&mut self, ip: I) { + match I::try_to_offset(ip, &self.reverse_bitmap_mapping) { + Ok(offset) => { + self.bitmap.set_ip_allocated(offset); + } + Err(e) => error!("Address {ip} cannot be retired from its pool: {e}"), + } + } + fn deallocate_from_pool(&mut self, ip: I) { debug!("Address {ip} was deallocated"); // The address was handed out by this pool, so it maps back into it. This runs while an diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 7a51a5e39e..1f250d33be 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -297,6 +297,130 @@ fn an_exhausted_region_falls_through_to_the_next() { ); } +/////////////////////////////////////////////////////////////////////////////// +// Exhaustion +/////////////////////////////////////////////////////////////////////////////// + +// Claiming every port masquerade could hand out on an address, which is how a test reaches +// address exhaustion without making 64k allocations. +fn claim_whole_address(offset: u128) -> PrefixWithOptionalPorts { + let address = Ipv4Addr::from(u32::try_from(BASE + offset).unwrap_or_else(|_| unreachable!())); + PrefixWithOptionalPorts::new( + format!("{address}/32").as_str().into(), + Some(PortRange::new(1024, u16::MAX).unwrap_or_else(|_| unreachable!())), + ) +} + +/// An address with nothing left to give is passed over, and the next one serves. +/// +/// Allocation draws the lowest free address and, finding no port on it, used to hand it straight +/// back. The same address was lowest next time, so the pool served nothing at all for as long as +/// it stayed there: one address fully claimed by port forwarding took a whole region out of +/// service. Claims on a later address never showed it, since allocation stopped before reaching +/// them. +#[test] +fn an_address_with_no_free_port_is_passed_over() { + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE + 2)], + idle_timeout: IDLE_TIMEOUT, + }]; + let claimed = PrefixPortsSet::from([claim_whole_address(0)]); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); + + // Repeatedly, so that an address taken out of the pool stays out. + let mut held = Vec::new(); + for _ in 0..4 { + let allocation = pool_sets[0] + .allocate(false) + .expect("two addresses of the region are free"); + assert_ne!( + allocation.ip(), + Ipv4Addr::from(u32::try_from(BASE).unwrap()), + "an address whose ports are all claimed was handed out" + ); + held.push(allocation); + } +} + +/// With every address claimed the pool has nothing to give, and says so rather than looping. +#[test] +fn a_fully_claimed_region_reports_exhaustion() { + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE + 2)], + idle_timeout: IDLE_TIMEOUT, + }]; + let claimed: PrefixPortsSet = (0..3).map(claim_whole_address).collect(); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); + + let error = pool_sets[0] + .allocate(false) + .expect_err("no address in the region can serve"); + assert!( + error.is_exhaustion(), + "a region with nothing to give reported {error} rather than being out of space" + ); +} + +/// An expose falls through to a region it shares once the region it has to itself is used up. +/// +/// The ordinary tests never reach this: a region is only given up when every port of every address +/// in it is taken, and allocation stays on one address for 64k ports, so a handful of allocations +/// never leaves the first address of the first region. Claiming the exclusive region away is how +/// the fallback gets exercised at all. +#[test] +fn an_expose_falls_back_to_shared_space_when_its_own_is_used_up() { + // Owner 0 has BASE..=BASE+1 to itself and shares BASE+2 with owner 1. + let specs = vec![ + PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE + 2)], + idle_timeout: IDLE_TIMEOUT, + }, + PoolSpec { + public_ranges: vec![AddrInterval::new(BASE + 2, BASE + 2)], + idle_timeout: IDLE_TIMEOUT, + }, + ]; + // Claim the exclusive region away, leaving owner 0 only the shared one. + let claimed: PrefixPortsSet = (0..2).map(claim_whole_address).collect(); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); + + let allocation = pool_sets[0] + .allocate(false) + .expect("the shared region still has room"); + assert_eq!( + allocation.ip(), + Ipv4Addr::from(u32::try_from(BASE + 2).unwrap()), + "the expose did not fall back to the region it shares" + ); +} + +/// Addresses are used up in turn, and every address of a region is reachable. +/// +/// Claims stand in for the 64k allocations it would otherwise take to move off an address, so the +/// walk over addresses is exercised at every depth rather than only at the first one. +#[test] +fn every_address_of_a_region_can_be_reached() { + const ADDRESSES: u128 = 6; + for claimed_count in 0..ADDRESSES { + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE + ADDRESSES - 1)], + idle_timeout: IDLE_TIMEOUT, + }]; + // Claim a prefix of the region away, so the first address left is the one after it. + let claimed: PrefixPortsSet = (0..claimed_count).map(claim_whole_address).collect(); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); + + let allocation = pool_sets[0].allocate(false).unwrap_or_else(|e| { + panic!("{claimed_count} addresses claimed, allocation failed: {e}") + }); + assert_eq!( + allocation.ip(), + Ipv4Addr::from(u32::try_from(BASE + claimed_count).unwrap()), + "with {claimed_count} addresses claimed the next one should serve" + ); + } +} + /////////////////////////////////////////////////////////////////////////////// // IPv6 /////////////////////////////////////////////////////////////////////////////// From 44862a61648e48efc4cb9e311742eb16ef414157 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 20:03:27 -0600 Subject: [PATCH 14/24] fix(masquerade): Keep the count of usable port blocks honest 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 7 +- nat/src/masquerade/apalloc/pool_fuzz.rs | 50 ++++++++++++ nat/src/masquerade/apalloc/port_alloc.rs | 97 +++++++++++++++--------- 3 files changed, 115 insertions(+), 39 deletions(-) diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index fbfd1250d3..7edad7633b 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -94,8 +94,11 @@ impl IpAllocator { outcome = Ok(port); break; } - // If there is no free port left, loop again to try another IP address - Err(AllocatorError::NoFreePort(_)) => {} + // This address has nothing left, whether it ran out of ports in a block or of + // blocks altogether. Either way the next address in hand may still have room, + // and giving up here would draw a fresh address while those sat with ports to + // spare. + Err(e) if e.is_exhaustion() => {} Err(e) => { outcome = Err(e); break; diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 1f250d33be..b613096f13 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -421,6 +421,56 @@ fn every_address_of_a_region_can_be_reached() { } } +/// An address that still has room is reused, rather than a fresh one being drawn. +/// +/// Reuse walks the addresses already in hand and skips those with nothing left. The skip is +/// decided by a count of blocks still free, and a block that port forwarding has claimed used to +/// be marked unusable without being taken off that count. The count then said an address had room +/// when it had none; the attempt failed with "no port block", and the walk gave up on that error +/// rather than trying the next address in hand. Every allocation after that drew a fresh address +/// while addresses already in hand sat with tens of thousands of free ports, until the region ran +/// out of addresses altogether. +#[test] +fn an_address_with_room_is_reused_before_a_fresh_one_is_drawn() { + const ADDRESSES: u128 = 4; + // Everything above the first block, so the first address has exactly one block: 1024..=1279. + let claim = PrefixWithOptionalPorts::new( + format!("{}/32", Ipv4Addr::from(u32::try_from(BASE).unwrap())) + .as_str() + .into(), + Some(PortRange::new(1280, u16::MAX).unwrap_or_else(|_| unreachable!())), + ); + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE + ADDRESSES - 1)], + idle_timeout: IDLE_TIMEOUT, + }]; + let pool_sets = pool_sets_for_specs::( + &specs, + &PrefixPortsSet::from([claim]), + NextHeader::TCP, + false, + ); + + // One block on the first address, then a handful more that the second address can serve + // many times over. + let mut held = Vec::new(); + let mut used = BTreeSet::new(); + for step in 0..(256 + 4) { + let allocation = pool_sets[0] + .allocate(false) + .unwrap_or_else(|e| panic!("allocation {step} failed: {e}")); + used.insert(allocation.ip()); + held.push(allocation); + } + + assert_eq!( + used.len(), + 2, + "the region spent {} addresses on what two can serve: {used:?}", + used.len() + ); +} + /////////////////////////////////////////////////////////////////////////////// // IPv6 /////////////////////////////////////////////////////////////////////////////// diff --git a/nat/src/masquerade/apalloc/port_alloc.rs b/nat/src/masquerade/apalloc/port_alloc.rs index e3d278f2b5..9d715bc182 100644 --- a/nat/src/masquerade/apalloc/port_alloc.rs +++ b/nat/src/masquerade/apalloc/port_alloc.rs @@ -94,9 +94,6 @@ pub(crate) struct PortAllocator { /// allocated by masquerade NAT for TCP or UDP. const IANA_WELLKNOWN_PORT_LIMIT: u16 = 1024; -/// Number of 256-port blocks covering the IANA well-known port range (0-1023). -const IANA_WELLKNOWN_BLOCKS: u16 = IANA_WELLKNOWN_PORT_LIMIT / 256; - impl PortAllocator { pub(crate) fn new( reserved_ports: PortClaims, @@ -112,22 +109,34 @@ impl PortAllocator { if randomize { Self::shuffle_slice(&mut base_ports); } - let blocks = std::array::from_fn(|i| { + let blocks: [AllocatorPortBlock; 256] = std::array::from_fn(|i| { let block = AllocatorPortBlock::new(base_ports[i]); - // Pre-mark IANA well-known port blocks (0-1023) as permanently non-free so they are - // never handed out by masquerade NAT for TCP or UDP. - if exclude_wellknown_ports && block.to_port_number() < IANA_WELLKNOWN_PORT_LIMIT { + let base = block.to_port_number(); + // Mark the blocks masquerade can never draw from as permanently non-free: the IANA + // well-known range (0-1023) for TCP and UDP, and any block port forwarding has claimed + // in full. Deciding this once, here, is what keeps the count of usable blocks honest: + // a block ruled out later would be marked non-free without ever being counted out. + if (exclude_wellknown_ports && base < IANA_WELLKNOWN_PORT_LIMIT) + || (!reserved_ports.is_empty() && reserved_ports.covers_block(base)) + { block .free .store(false, concurrency::sync::atomic::Ordering::Relaxed); } block }); - let usable_blocks = if exclude_wellknown_ports { - 256 - IANA_WELLKNOWN_BLOCKS - } else { - 256 - }; + // Counted from the blocks themselves rather than assumed, so the two cannot disagree. + let usable_blocks = u16::try_from( + blocks + .iter() + .filter(|block| { + block + .free + .load(concurrency::sync::atomic::Ordering::Relaxed) + }) + .count(), + ) + .unwrap_or(u16::MAX); Self { blocks, usable_blocks: AtomicU16::new(usable_blocks), @@ -195,13 +204,19 @@ impl PortAllocator { // finding a weak reference that won't upgrade. Removing here would require an additional // lookup in the list. // - // TODO: Should we move usable_blocks and blocks into a lock-protected struct? Or adjust the - // ordering for the atomic operations? + // TODO: Should we move usable_blocks and blocks into a lock-protected struct? + // + // The count is given back before the flag, and the order matters. A claimant subtracts + // from the count only after winning the flag, so raising the flag first leaves a window in + // which the count is still short by this block and someone else's subtraction takes it + // below zero -- on a `u16`, to 65535, which says the address has room it does not have. + // This way round the count is briefly one too high instead, which only says an address has + // room a moment before it truly does. + self.usable_blocks + .fetch_add(1, concurrency::sync::atomic::Ordering::Relaxed); self.blocks[index] .free .store(true, concurrency::sync::atomic::Ordering::Relaxed); - self.usable_blocks - .fetch_add(1, concurrency::sync::atomic::Ordering::Relaxed); } fn has_allocated_blocks_with_free_ports(&self) -> bool { @@ -209,13 +224,17 @@ impl PortAllocator { } // Find an available block to allocate ports from, and mark it as non-free. + // + // The blocks masquerade may never draw from, the well-known range and those port forwarding + // has claimed in full, were marked non-free when the allocator was built, so taking the first + // block whose flag can be claimed is the whole of the decision. fn pick_available_block(&self) -> Result<(usize, u16), AllocatorError> { - // Find the first free block in the list, starting from the current self.current_alloc_index + // Starting from the current self.current_alloc_index, take the first block for which the + // atomic compare_exchange succeeds. let (index, block) = self .cycle_blocks() .find(|(_, block)| { - // Find the first block for which the atomic compare_exchange succeeds - if block + block .free .compare_exchange( true, @@ -223,16 +242,7 @@ impl PortAllocator { concurrency::sync::atomic::Ordering::Relaxed, concurrency::sync::atomic::Ordering::Relaxed, ) - .is_err() - { - return false; - } - - // Skip a block port forwarding has claimed in full: there is nothing left in it to - // hand out. Several claims may cover a block between them while no single one of - // them does, so this asks the claims as a whole rather than testing them one by - // one. - !self.reserved_ports.covers_block(block.to_port_number()) + .is_ok() }) .ok_or(AllocatorError::NoPortBlock)?; Ok((index, block.to_port_number())) @@ -261,7 +271,13 @@ impl PortAllocator { let reserved_for_block: Vec = self.reserved_ports.within_block(base_port_index).collect(); + // Building the block is the last thing that can fail, and by here its flag is taken and + // the count is down. Nothing would give either back: no `Arc` exists yet, so no `Drop` is + // coming, and the block would sit claimed by nobody for the life of the allocator. Only + // reachable through an `InternalIssue` that should not happen, which is exactly the sort + // of thing that turns one bad block into a slow leak. AllocatedPortBlock::new(ip, index, base_port_index, &reserved_for_block, allow_null) + .inspect_err(|_| self.deallocate_block(index)) } pub(crate) fn allocate_port( @@ -325,13 +341,12 @@ impl PortAllocator { // other, so it carries the same claims, clipped to it. let reserved_for_block: Vec = self.reserved_ports.within_block(base_port_index).collect(); - let block = Arc::new(AllocatedPortBlock::new( - ip, - index, - base_port_index, - &reserved_for_block, - allow_null, - )?); + // As in `allocate_block`: the flag is already taken and the count already down, and only + // a `Drop` gives them back. There is no `Arc` to drop if this fails. + let block = Arc::new( + AllocatedPortBlock::new(ip, index, base_port_index, &reserved_for_block, allow_null) + .inspect_err(|_| self.deallocate_block(index))?, + ); self.allocated_blocks .insert(block.index, Arc::downgrade(&block)); Ok(block) @@ -367,7 +382,15 @@ impl PortAllocator { // Reject explicit reservations into the IANA system/well-known range up front so callers // get a policy-oriented error rather than a misleading resource-exhaustion error from the // pre-excluded low-port blocks. - if self.exclude_wellknown_ports && port.as_u16() < IANA_WELLKNOWN_PORT_LIMIT { + // + // The range is a port-number convention, so it says nothing about an ICMP identifier, + // which is drawn from the whole 16 bits. Identifiers cannot meet this today -- ICMP pools + // are built with the exclusion off -- but that is decided in `setup.rs`, and a reader here + // would have to go and find it to know a low identifier is not silently denied. + if self.exclude_wellknown_ports + && !matches!(port, NatPort::Identifier(_)) + && port.as_u16() < IANA_WELLKNOWN_PORT_LIMIT + { debug!("Explicit reservation for well-known port {port} denied by allocator policy"); return Err(AllocatorError::Denied); } From 2053688da625efa1c271fa8e63006aa55321b448 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 21:15:46 -0600 Subject: [PATCH 15/24] fix(masquerade): Tell a block claimed in full apart from a bookkeeping 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/port_alloc.rs | 45 ++++++++++++++++++------ 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/nat/src/masquerade/apalloc/port_alloc.rs b/nat/src/masquerade/apalloc/port_alloc.rs index 9d715bc182..c829b39ff4 100644 --- a/nat/src/masquerade/apalloc/port_alloc.rs +++ b/nat/src/masquerade/apalloc/port_alloc.rs @@ -352,6 +352,17 @@ impl PortAllocator { Ok(block) } + // Whether the block holding this port is one masquerade may never draw from, and so was marked + // non-free when the allocator was built without ever having been allocated. + // + // Mirrors the condition in `new`, and has to keep mirroring it: a block ruled out there but not + // recognized here is taken for a live allocation that has gone missing. + fn block_is_excluded(&self, port: NatPort) -> bool { + let base = (port.as_u16() / 256) * 256; + (self.exclude_wellknown_ports && base < IANA_WELLKNOWN_PORT_LIMIT) + || self.reserved_ports.covers_block(base) + } + fn find_block_for_port( &self, ip: Arc>, @@ -362,16 +373,30 @@ impl PortAllocator { if block_was_free { return self.allocate_block_for_reservation(ip, index, port, allow_null); } - self.allocated_blocks - .search_for_block(port) - // Block was not free but is not in the list of allocated blocks either?? - // - // FIXME: This can legitimately happen if the block was released just after we checked - // whether it was free? (Not observed in shuttle tests so far.) Do we need an additional - // lock around the PortAllocator? - .ok_or(AllocatorError::InternalIssue( - "Block not free, although absent from list of allocated blocks".to_string(), - )) + if let Some(block) = self.allocated_blocks.search_for_block(port) { + return Ok(block); + } + // A block masquerade may never draw from never joins the list of allocated blocks, so its + // absence from that list says nothing about the bookkeeping. Port forwarding claiming a + // block in full is the way to reach this from a configuration: a flow carried across a + // config change may hold a port in a block the new configuration has claimed. + // + // Answer as a claim on part of the same block does, where the block is allocatable and its + // own bitmap refuses the port. How much of a block an operator happened to claim is not + // something the caller should be able to tell apart, and it is certainly not the difference + // between a policy conflict and a broken allocator. + if self.block_is_excluded(port) { + debug!("Port {port} lies in a block that port forwarding has claimed in full"); + return Err(AllocatorError::PortReservationFailed(port.as_u16())); + } + // Block was not free but is not in the list of allocated blocks either?? + // + // FIXME: This can legitimately happen if the block was released just after we checked + // whether it was free? (Not observed in shuttle tests so far.) Do we need an additional + // lock around the PortAllocator? + Err(AllocatorError::InternalIssue( + "Block not free, although absent from list of allocated blocks".to_string(), + )) } pub(crate) fn reserve_port( From d5ac16a15213693e7690ab3f69fe858c7bd58411 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 22:10:28 -0600 Subject: [PATCH 16/24] fix(masquerade): Keep addresses port forwarding has used up out of the 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/alloc.rs | 61 ++++++- nat/src/masquerade/apalloc/pool_fuzz.rs | 99 ++++++++++- nat/src/masquerade/apalloc/port_alloc.rs | 14 +- nat/src/masquerade/apalloc/reserved.rs | 213 +++++++++++++++++++++++ 4 files changed, 370 insertions(+), 17 deletions(-) diff --git a/nat/src/masquerade/apalloc/alloc.rs b/nat/src/masquerade/apalloc/alloc.rs index 7edad7633b..63ba68b3fa 100644 --- a/nat/src/masquerade/apalloc/alloc.rs +++ b/nat/src/masquerade/apalloc/alloc.rs @@ -30,7 +30,7 @@ use tracing::{debug, error}; /// ordinary case, where the first address serves, and an address that cannot serve is taken out of /// the pool as it is found, so a run of them is worked through over successive packets rather than /// being walked again each time. -const MAX_ADDRESSES_PER_ALLOCATION: usize = 8; +pub(super) const MAX_ADDRESSES_PER_ALLOCATION: usize = 8; /////////////////////////////////////////////////////////////////////////////// // IpAllocator @@ -410,6 +410,16 @@ impl Drop for AllocatedIp { #[derive(Debug)] pub(crate) struct NatPool { bitmap: PoolBitmap, + /// Offsets that may never be handed out, however often they are given back. + /// + /// Distinct from being absent from `bitmap`, which only says an address is in use at the + /// moment. An address lands here either because port forwarding has claimed every port + /// masquerade could draw on it, which is known when the pool is built, or because it was found + /// to have nothing to give while serving. Either way it must not come back: + /// `deallocate_from_pool` runs on the drop path and would otherwise return it to the pool the + /// first time a flow holding it ends, which for a carried-over flow is moments after the pool + /// was built. + unusable: RoaringBitmap, bitmap_mapping: BTreeMap, reverse_bitmap_mapping: BTreeMap, in_use: VecDeque>>, @@ -437,16 +447,41 @@ impl NatPool { // A region holding more addresses than the u32 bitmap can index is truncated. We would run // out of memory long before allocating four billion addresses. let span = range.len().saturating_sub(1).min(u128::from(u32::MAX)); + let indexable = AddrInterval::new(range.start, range.start + span); let to_offset = |bits: u128| { let address = I::try_from_bits(bits).unwrap_or_else(|()| unreachable!()); I::try_to_offset(address, &reverse_bitmap_mapping).unwrap_or_else(|_| unreachable!()) }; + let mut bitmap = + PoolBitmap::with_offset_range(to_offset(indexable.start), to_offset(indexable.end)); + + // An address port forwarding has taken every usable port on can serve masquerade nothing, + // and which addresses those are is known now rather than discovered a packet at a time. + // Left in the pool, such an address is drawn because it is the lowest one free, found + // useless, and taken out -- but only eight of them may be worked through before the + // allocation gives up, so a run of them costs a dropped packet for every eight, on flows + // the region had ample room for. And it costs them again after every config change, since + // a new allocator starts with a fresh pool. Keeping them out from the start costs one + // sweep over the claims when the pool is built. + let mut unusable = RoaringBitmap::new(); + for interval in reserved_ports.unusable_within::(indexable, exclude_wellknown_ports) { + let (first, last) = (to_offset(interval.start), to_offset(interval.end)); + unusable.insert_range(first..=last); + bitmap.remove_offset_range(first, last); + } + if !unusable.is_empty() { + debug!( + "Pool over {} address(es) keeps {} of them out: port forwarding has claimed every \ + port masquerade could use there", + indexable.len(), + unusable.len() + ); + } + Self { - bitmap: PoolBitmap::with_offset_range( - to_offset(range.start), - to_offset(range.start + span), - ), + bitmap, + unusable, bitmap_mapping, reverse_bitmap_mapping, in_use: VecDeque::new(), @@ -509,6 +544,7 @@ impl NatPool { fn retire_from_pool(&mut self, ip: I) { match I::try_to_offset(ip, &self.reverse_bitmap_mapping) { Ok(offset) => { + self.unusable.insert(offset); self.bitmap.set_ip_allocated(offset); } Err(e) => error!("Address {ip} cannot be retired from its pool: {e}"), @@ -522,7 +558,15 @@ impl NatPool { // address marked in use rather than panicking on the drop path. match I::try_to_offset(ip, &self.reverse_bitmap_mapping) { Ok(offset) => { - self.bitmap.set_ip_free(offset); + // An address that can never serve does not come back, whoever is giving it up. + // Reserving reaches addresses the pool never draws, so a flow carried across a + // config change onto an address the new configuration has claimed would otherwise + // put it into the pool on its way out. + if self.unusable.contains(offset) { + debug!("Address {ip} stays out of its pool: it can serve nothing"); + } else { + self.bitmap.set_ip_free(offset); + } } Err(e) => error!("Address {ip} does not map back into the pool it came from: {e}"), } @@ -657,6 +701,11 @@ impl PoolBitmap { fn set_ip_free(&mut self, index: u32) -> bool { self.0.insert(index) } + + /// Take an inclusive range of indices out of the pool. + fn remove_offset_range(&mut self, start: u32, end: u32) { + self.0.remove_range(start..=end); + } } /////////////////////////////////////////////////////////////////////////////// diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index b613096f13..2d8512842a 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -19,7 +19,7 @@ #![cfg(test)] -use super::alloc::PoolSet; +use super::alloc::{MAX_ADDRESSES_PER_ALLOCATION, PoolSet}; use super::region::AddrInterval; use super::setup::{PoolSpec, pool_sets_for_specs}; use crate::masquerade::allocation::AllocatorError; @@ -116,6 +116,10 @@ fn declares(ranges: &[AddrInterval], ip: Ipv4Addr) -> bool { ranges.iter().any(|range| range.contains(bits(ip))) } +fn port(value: u16) -> NatPort { + NatPort::new_port_checked(value).unwrap_or_else(|_| unreachable!()) +} + /// Allocate round-robin across the exposes, holding every allocation so nothing is freed and /// reused mid-run. Returns which expose got what. fn allocate_round_robin( @@ -421,6 +425,99 @@ fn every_address_of_a_region_can_be_reached() { } } +/// A run of claimed addresses costs nothing, however long it is. +/// +/// Allocation draws the lowest free address, so a port-forwarded prefix at the bottom of a region +/// is what it meets first. Those addresses used to sit in the pool: each was drawn, found to have +/// no port to give, and taken out again, and an allocation may only work through +/// `MAX_ADDRESSES_PER_ALLOCATION` of them before it gives up. A run therefore shed a dropped packet +/// for every bound's worth of it -- sixteen for a claimed `/25` -- on flows the region had tens of +/// thousands of ports waiting for. And it shed them again after every config change, since a new +/// allocator starts with a fresh pool and rediscovers the whole run. +/// +/// They are kept out of the pool when it is built now, so the first allocation serves whatever the +/// length of the run. The bound stays for the case a configuration cannot produce: an address +/// emptied by another thread between being drawn and being drawn upon. +#[test] +fn a_run_of_claimed_addresses_costs_no_allocations() { + let bound = u128::try_from(MAX_ADDRESSES_PER_ALLOCATION).unwrap_or_else(|_| unreachable!()); + + // Around the old cliff, and then well past it: the last of these used to cost three packets. + for claimed_count in [0, 1, bound - 1, bound, bound + 1, 3 * bound + 5] { + let specs = vec![PoolSpec { + // One address past the claimed run, which is the one that has to serve. + public_ranges: vec![AddrInterval::new(BASE, BASE + claimed_count)], + idle_timeout: IDLE_TIMEOUT, + }]; + let claimed: PrefixPortsSet = (0..claimed_count).map(claim_whole_address).collect(); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); + + let allocation = pool_sets[0].allocate(false).unwrap_or_else(|e| { + panic!("a run of {claimed_count} claimed addresses cost the first allocation: {e}") + }); + assert_eq!( + allocation.ip(), + Ipv4Addr::from(u32::try_from(BASE + claimed_count).unwrap_or_else(|_| unreachable!())), + "the address past a run of {claimed_count} claimed ones should be the one that serves" + ); + } +} + +/// An address the pool may never draw is not put into it by a flow that fails to carry over. +/// +/// Reserving reaches addresses allocation never touches. A flow that survives a config change +/// presents the address it already holds, and the pool takes that address into use in order to try +/// to give the port back. Where the new configuration has claimed every port on it the reservation +/// fails, as it must -- but the address has been through the pool by then, and releasing it on the +/// way out used to hand it to the bitmap. The exclusion would undo itself on the first config +/// change that needed it, which is also the first one that could produce such a flow. +#[test] +fn a_failed_carry_over_does_not_put_a_claimed_address_into_the_pool() { + const ADDRESSES: u128 = 4; + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE + ADDRESSES - 1)], + idle_timeout: IDLE_TIMEOUT, + }]; + // The lowest two addresses are claimed end to end, so neither may ever be handed out. + let claimed: PrefixPortsSet = (0..2).map(claim_whole_address).collect(); + let pool_sets = pool_sets_for_specs::(&specs, &claimed, NextHeader::TCP, false); + let claimed_address = Ipv4Addr::from(u32::try_from(BASE).unwrap_or_else(|_| unreachable!())); + + let free_offsets = || { + let region = pool_sets[0] + .regions() + .next() + .expect("the specs describe one region"); + let (bitmap, _) = region.allocator().get_pool_clone_for_tests(); + bitmap + }; + assert!( + !free_offsets().contains(claimed_address.to_bits()), + "a fully claimed address was in the pool to begin with" + ); + + // Carry a flow over onto it. Refused, and the address goes back where it came from. + assert!( + pool_sets[0].reserve(claimed_address, port(5000)).is_err(), + "a port on a fully claimed address was carried over" + ); + + assert!( + !free_offsets().contains(claimed_address.to_bits()), + "a fully claimed address was put into the pool by a carry-over that failed on it" + ); + + // And allocation still goes straight to the first address that can serve. + let allocation = pool_sets[0] + .allocate(false) + .expect("the region has addresses that can serve"); + assert_eq!( + allocation.ip(), + Ipv4Addr::from(u32::try_from(BASE + 2).unwrap_or_else(|_| unreachable!())), + "allocation did not go straight past the claimed addresses" + ); +} + /// An address that still has room is reused, rather than a fresh one being drawn. /// /// Reuse walks the addresses already in hand and skips those with nothing left. The skip is diff --git a/nat/src/masquerade/apalloc/port_alloc.rs b/nat/src/masquerade/apalloc/port_alloc.rs index c829b39ff4..b3ca879aed 100644 --- a/nat/src/masquerade/apalloc/port_alloc.rs +++ b/nat/src/masquerade/apalloc/port_alloc.rs @@ -10,7 +10,7 @@ use super::NatIpWithBitmap; use super::alloc::AllocatedIp; -use super::reserved::PortClaims; +use super::reserved::{IANA_WELLKNOWN_PORT_LIMIT, PortClaims}; use crate::masquerade::allocation::AllocatorError; use crate::port::NatPort; use concurrency::concurrency_mode; @@ -90,10 +90,6 @@ pub(crate) struct PortAllocator { exclude_wellknown_ports: bool, } -/// Ports 0..=1023 cover the IANA system/well-known range and should not be -/// allocated by masquerade NAT for TCP or UDP. -const IANA_WELLKNOWN_PORT_LIMIT: u16 = 1024; - impl PortAllocator { pub(crate) fn new( reserved_ports: PortClaims, @@ -116,9 +112,7 @@ impl PortAllocator { // well-known range (0-1023) for TCP and UDP, and any block port forwarding has claimed // in full. Deciding this once, here, is what keeps the count of usable blocks honest: // a block ruled out later would be marked non-free without ever being counted out. - if (exclude_wellknown_ports && base < IANA_WELLKNOWN_PORT_LIMIT) - || (!reserved_ports.is_empty() && reserved_ports.covers_block(base)) - { + if reserved_ports.block_is_unusable(base, exclude_wellknown_ports) { block .free .store(false, concurrency::sync::atomic::Ordering::Relaxed); @@ -359,8 +353,8 @@ impl PortAllocator { // recognized here is taken for a live allocation that has gone missing. fn block_is_excluded(&self, port: NatPort) -> bool { let base = (port.as_u16() / 256) * 256; - (self.exclude_wellknown_ports && base < IANA_WELLKNOWN_PORT_LIMIT) - || self.reserved_ports.covers_block(base) + self.reserved_ports + .block_is_unusable(base, self.exclude_wellknown_ports) } fn find_block_for_port( diff --git a/nat/src/masquerade/apalloc/reserved.rs b/nat/src/masquerade/apalloc/reserved.rs index 941a99870a..77c8e94a05 100644 --- a/nat/src/masquerade/apalloc/reserved.rs +++ b/nat/src/masquerade/apalloc/reserved.rs @@ -13,10 +13,20 @@ //! claims on an address are kept as a list and every one of them applies. Holding a single range //! per address instead would silently honour whichever was recorded last. +use super::region::AddrInterval; +use crate::masquerade::natip::NatIp; use crate::ranges::IpRange; use lpm::prefix::PortRange; +use std::collections::BTreeSet; use std::net::IpAddr; +/// Ports 0..=1023 cover the IANA system/well-known range and should not be +/// allocated by masquerade NAT for TCP or UDP. +pub(crate) const IANA_WELLKNOWN_PORT_LIMIT: u16 = 1024; + +/// How many 256-port blocks an address is divided into. +const BLOCKS_PER_ADDRESS: u32 = 256; + /// Ports claimed on public addresses, as a flat list of claims. /// /// Kept flat rather than keyed by address, because claims may be made on overlapping address @@ -51,6 +61,69 @@ impl ReservedPorts { pub(crate) fn iter(&self) -> impl Iterator + '_ { self.claims.iter().copied() } + + /// The stretches of `range` on which masquerade could never hand anything out, because port + /// forwarding has claimed every port it is allowed to draw from. + /// + /// Walks the claims rather than the addresses: a region may hold billions of addresses, while + /// there are only as many claims as there are port-forwarding exposes towards one peer. + /// Coverage can only change where a claim begins or just past where one ends, so cutting at + /// those points gives stretches over which the answer cannot change and a single address + /// decides each of them. Adjacent stretches that agree are merged. + /// + /// `range` must be one every address of which converts to an offset, so callers pass the + /// indexable part of a region rather than the whole of it. + pub(crate) fn unusable_within( + &self, + range: AddrInterval, + exclude_wellknown_ports: bool, + ) -> Vec { + // With nothing claimed there is nothing to find: the well-known range on its own never + // uses an address up, since every block above it is still there to draw from. + if self.claims.is_empty() { + return Vec::new(); + } + + let mut cuts = BTreeSet::from([range.start]); + for (addresses, _) in &self.claims { + let Some((start, end)) = claim_bounds::(addresses) else { + // A claim of the other address family, which says nothing about this region. + continue; + }; + for cut in [Some(start), end.checked_add(1)].into_iter().flatten() { + if cut > range.start && cut <= range.end { + cuts.insert(cut); + } + } + } + + let cuts: Vec = cuts.into_iter().collect(); + let mut unusable: Vec = Vec::new(); + for (index, &start) in cuts.iter().enumerate() { + let end = cuts.get(index + 1).map_or(range.end, |&next| next - 1); + let Ok(address) = I::try_from_bits(start) else { + continue; + }; + if !self + .for_address(address.to_ip_addr()) + .every_block_is_unusable(exclude_wellknown_ports) + { + continue; + } + match unusable.last_mut() { + Some(previous) if previous.end.checked_add(1) == Some(start) => previous.end = end, + _ => unusable.push(AddrInterval::new(start, end)), + } + } + unusable + } +} + +// The bounds of a claim as raw bits, or None if the claim is of another address family. +fn claim_bounds(addresses: &IpRange) -> Option<(u128, u128)> { + let start = I::try_from_addr(addresses.start()).ok()?; + let end = I::try_from_addr(addresses.end()).ok()?; + Some((start.to_addr_bits(), end.to_addr_bits())) } /// The port ranges claimed on one public address. @@ -81,6 +154,29 @@ impl PortClaims { .filter_map(move |claim| claim.intersection(block)) } + /// Whether the block at `base` is one masquerade may never draw from, whether because policy + /// keeps it off or because port forwarding has taken the whole of it. + /// + /// This is the decision the port allocator makes when it marks a block permanently non-free, + /// and the one [`every_block_is_unusable`](Self::every_block_is_unusable) asks 256 times over. + /// Both go through here so that the address-level answer cannot drift from the block-level one: + /// an address dropped from a pool while a block of it was still allocatable would be capacity + /// silently thrown away. + pub(crate) fn block_is_unusable(&self, base: u16, exclude_wellknown_ports: bool) -> bool { + (exclude_wellknown_ports && base < IANA_WELLKNOWN_PORT_LIMIT) + || (!self.is_empty() && self.covers_block(base)) + } + + /// Whether no block of the address carrying these claims can be drawn from, so the address can + /// serve masquerade nothing at all. + pub(crate) fn every_block_is_unusable(&self, exclude_wellknown_ports: bool) -> bool { + (0..BLOCKS_PER_ADDRESS) + .map(|index| { + u16::try_from(index * 256).unwrap_or_else(|_| unreachable!("256 blocks of 256")) + }) + .all(|base| self.block_is_unusable(base, exclude_wellknown_ports)) + } + /// Whether every port the block at `base` could hand out is claimed, so the block is of no use /// to masquerade at all. /// @@ -123,6 +219,123 @@ fn block_range(base: u16) -> PortRange { PortRange::new(base, base.saturating_add(255)).unwrap_or_else(|_| unreachable!()) } +#[cfg(test)] +mod bolero_tests { + use super::*; + use bolero::{Driver, TypeGenerator}; + use std::net::Ipv4Addr; + + // A narrow window, so that claims overlap one another as a matter of course and every address + // in it can be checked rather than a sampled few. + const BASE: u32 = 0x0A01_0000; + const WINDOW: u32 = 12; + const MAX_CLAIMS: u8 = 4; + + /// A generated set of claims: each an offset and a length into the window, and a port range + /// that is either the whole of what masquerade could draw on or some slice of it. + #[derive(Debug, Clone)] + struct Scenario { + claims: Vec<(u8, u8, bool, u16, u16)>, + } + + impl TypeGenerator for Scenario { + fn generate(driver: &mut D) -> Option { + let count = usize::from(driver.produce::()? % (MAX_CLAIMS + 1)); + let mut claims = Vec::with_capacity(count); + for _ in 0..count { + claims.push(( + driver.produce::()?, + driver.produce::()?, + // Claims over the whole usable port space are the only ones that can use an + // address up on their own. Drawn deliberately, because a range that happens to + // reach from 1024 to 65535 is otherwise vanishingly rare. + driver.produce::()?, + driver.produce::()?, + driver.produce::()?, + )); + } + Some(Self { claims }) + } + } + + impl Scenario { + fn reserved(&self) -> ReservedPorts { + let mut reserved = ReservedPorts::default(); + for &(offset, length, whole, port_lo, port_span) in &self.claims { + let start = BASE + u32::from(offset) % WINDOW; + // Claims may reach past the end of the window, which the clipping has to survive. + let end = start + u32::from(length) % WINDOW; + let ports = if whole { + PortRange::new(IANA_WELLKNOWN_PORT_LIMIT, u16::MAX) + } else { + let low = port_lo.max(1); + PortRange::new(low, low.saturating_add(port_span)) + } + .unwrap_or_else(|_| unreachable!()); + reserved.claim( + IpRange::new( + IpAddr::V4(Ipv4Addr::from(start)), + IpAddr::V4(Ipv4Addr::from(end)), + ), + ports, + ); + } + reserved + } + } + + #[test] + fn unusable_within_matches_a_per_address_oracle() { + bolero::check!() + .with_type() + .cloned() + .for_each(|scenario: Scenario| { + let reserved = scenario.reserved(); + let range = AddrInterval::new(u128::from(BASE), u128::from(BASE + WINDOW - 1)); + let unusable = reserved.unusable_within::(range, true); + + for interval in &unusable { + assert!( + interval.start <= interval.end, + "interval {interval:?} ends before it starts" + ); + assert!( + interval.start >= range.start && interval.end <= range.end, + "interval {interval:?} reaches outside the range it was asked about" + ); + } + + // Ordered, and never touching: two that met should have been reported as one. + for pair in unusable.windows(2) { + assert!( + pair[0].end.saturating_add(1) < pair[1].start, + "intervals {:?} and {:?} touch, overlap, or are out of order", + pair[0], + pair[1] + ); + } + + // The load-bearing property, at every address in the window. An address is kept out + // of the pool exactly when no block of it could ever be drawn from: keeping out one + // that still had a block is capacity silently thrown away, and leaving in one that + // had none is the walk this exists to avoid. + for bits in range.start..=range.end { + let address = + Ipv4Addr::from(u32::try_from(bits).unwrap_or_else(|_| unreachable!())); + let has_nothing_to_give = reserved + .for_address(IpAddr::V4(address)) + .every_block_is_unusable(true); + let kept_out = unusable.iter().any(|interval| interval.contains(bits)); + assert_eq!( + kept_out, has_nothing_to_give, + "address {address}: kept out of the pool = {kept_out}, but every block \ + unusable = {has_nothing_to_give}" + ); + } + }); + } +} + #[cfg(test)] mod tests { use super::*; From 99eb70a64109b665ada83a6e46338ef0f0109a91 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 22:13:45 -0600 Subject: [PATCH 17/24] test(masquerade): Give the unusable-address property a real oracle 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/reserved.rs | 65 ++++++++++++++++++++------ 1 file changed, 51 insertions(+), 14 deletions(-) diff --git a/nat/src/masquerade/apalloc/reserved.rs b/nat/src/masquerade/apalloc/reserved.rs index 77c8e94a05..d0b58c00f5 100644 --- a/nat/src/masquerade/apalloc/reserved.rs +++ b/nat/src/masquerade/apalloc/reserved.rs @@ -235,7 +235,7 @@ mod bolero_tests { /// that is either the whole of what masquerade could draw on or some slice of it. #[derive(Debug, Clone)] struct Scenario { - claims: Vec<(u8, u8, bool, u16, u16)>, + claims: Vec<(u8, u8, u8, u16, u16)>, } impl TypeGenerator for Scenario { @@ -246,10 +246,10 @@ mod bolero_tests { claims.push(( driver.produce::()?, driver.produce::()?, - // Claims over the whole usable port space are the only ones that can use an - // address up on their own. Drawn deliberately, because a range that happens to - // reach from 1024 to 65535 is otherwise vanishingly rare. - driver.produce::()?, + // Which shape of port range to draw. Left to chance, a range that happens to + // reach from 1024 to 65535, or to stop exactly on a block boundary, is + // vanishingly rare, and those are the shapes the answer turns on. + driver.produce::()?, driver.produce::()?, driver.produce::()?, )); @@ -261,15 +261,31 @@ mod bolero_tests { impl Scenario { fn reserved(&self) -> ReservedPorts { let mut reserved = ReservedPorts::default(); - for &(offset, length, whole, port_lo, port_span) in &self.claims { + for &(offset, length, shape, port_lo, port_span) in &self.claims { let start = BASE + u32::from(offset) % WINDOW; // Claims may reach past the end of the window, which the clipping has to survive. let end = start + u32::from(length) % WINDOW; - let ports = if whole { - PortRange::new(IANA_WELLKNOWN_PORT_LIMIT, u16::MAX) - } else { - let low = port_lo.max(1); - PortRange::new(low, low.saturating_add(port_span)) + let ports = match shape % 3 { + // The whole of what masquerade could draw on, which uses the address up by + // itself. + 0 => PortRange::new(IANA_WELLKNOWN_PORT_LIMIT, u16::MAX), + // Up to the end of some block: the shape that decides whether the block a + // claim stops on is judged the same way by the address-level answer and the + // block-level one. + 1 => { + let blocks = 1 + u32::from(port_lo) % 255; + let end = (u32::from(IANA_WELLKNOWN_PORT_LIMIT) + blocks * 256 - 1) + .min(u32::from(u16::MAX)); + PortRange::new( + IANA_WELLKNOWN_PORT_LIMIT, + u16::try_from(end).unwrap_or_else(|_| unreachable!()), + ) + } + // Anywhere at all. + _ => { + let low = port_lo.max(1); + PortRange::new(low, low.saturating_add(port_span)) + } } .unwrap_or_else(|_| unreachable!()); reserved.claim( @@ -322,9 +338,8 @@ mod bolero_tests { for bits in range.start..=range.end { let address = Ipv4Addr::from(u32::try_from(bits).unwrap_or_else(|_| unreachable!())); - let has_nothing_to_give = reserved - .for_address(IpAddr::V4(address)) - .every_block_is_unusable(true); + let has_nothing_to_give = + covers_every_usable_port(&reserved.for_address(IpAddr::V4(address))); let kept_out = unusable.iter().any(|interval| interval.contains(bits)); assert_eq!( kept_out, has_nothing_to_give, @@ -334,6 +349,28 @@ mod bolero_tests { } }); } + + /// The oracle: an address can serve nothing when the claims on it cover every port masquerade + /// could hand out, end to end. + /// + /// Worked out over the port space directly, rather than by asking whether each of the 256 + /// blocks is covered. That is the whole value of it: the implementation deliberately routes the + /// address-level answer and the block-level one through a single predicate so they cannot + /// drift, and a test that reused that predicate would be comparing it with itself. + fn covers_every_usable_port(claims: &PortClaims) -> bool { + let mut ranges: Vec = claims.iter().collect(); + ranges.sort_by_key(PortRange::start); + + // Everything below the well-known limit is off the table anyway, so coverage starts there. + let mut covered_through = IANA_WELLKNOWN_PORT_LIMIT - 1; + for range in ranges { + if range.start() > covered_through.saturating_add(1) { + return false; + } + covered_through = covered_through.max(range.end()); + } + covered_through == u16::MAX + } } #[cfg(test)] From c03e7f54c251af4ff74649ef6d11df88bce452c9 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 00:22:10 -0600 Subject: [PATCH 18/24] test(masquerade): Model the masquerade-over-forwarded-ports fixture on 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/test_alloc.rs | 32 +++++++++++------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/nat/src/masquerade/apalloc/test_alloc.rs b/nat/src/masquerade/apalloc/test_alloc.rs index d0afb50cdb..5618db4793 100644 --- a/nat/src/masquerade/apalloc/test_alloc.rs +++ b/nat/src/masquerade/apalloc/test_alloc.rs @@ -339,10 +339,11 @@ mod context { NatAllocator::new(config) } - // VPC-1 masquerades onto a single public address towards VPC-3, while VPC-2 port-forwards a - // range of ports on that same public address towards the same VPC-3. The public space towards - // a peer is shared, so those ports are spoken for and masquerade may not hand them out: - // return traffic to one of them carries nothing that says which of the two it belongs to. + // VPC-1 masquerades onto a single public address towards VPC-3, and forwards a range of + // ports on that same address towards the same peer, both declared by the one manifest. The + // public space towards a peer is shared between every expose that reaches it, so those ports + // are spoken for and masquerade may not hand them out: return traffic to one of them carries + // nothing that says which expose it belongs to. #[allow(dead_code)] fn build_context_masquerade_over_forwarded_ports() -> ValidatedVpcTable { let masquerade = VpcExpose::empty() @@ -351,7 +352,13 @@ mod context { .ip("1.1.0.0/16".into()) .as_range("10.1.0.0/32".into()) .unwrap(); - // The forwarded ports sit on the very address VPC-1 masquerades onto. + // The forwarded ports sit on the very address VPC-1 masquerades onto, declared by the + // same manifest. That is the shape validation accepts: masquerade and port forwarding may + // overlap within one manifest, each implying a direction, while the same overlap across + // two VPCs' peerings is rejected by the peer's route table, which could not route the + // shared address to one peering. An earlier version of this fixture used the cross-VPC + // shape and passed only because building a VpcTable by hand skips collecting peerings + // into the peer, so the route check never saw it. let forwarded = VpcExpose::empty() .make_port_forwarding(None, None) .unwrap() @@ -368,21 +375,11 @@ mod context { VpcManifest::with_exposes("VPC-3", vec![VpcExpose::empty().ip("3.0.0.0/24".into())]); let mut vpc1 = Vpc::new("VPC-1", "67890", vni1().as_u32()).unwrap(); - let mut vpc2 = Vpc::new("VPC-2", "12345", vni2().as_u32()).unwrap(); let vpc3 = Vpc::new("VPC-3", "11111", vni3().as_u32()).unwrap(); vpc1.peerings.push(Peering { name: "masquerading_peering".into(), - local: VpcManifest::with_exposes("VPC-1", vec![masquerade]), - remote: remote.clone(), - remote_id: "11111".try_into().unwrap(), - remote_vni: vpc3.vni, - gwgroup: "default".into(), - acl: None, - }); - vpc2.peerings.push(Peering { - name: "forwarding_peering".into(), - local: VpcManifest::with_exposes("VPC-2", vec![forwarded]), + local: VpcManifest::with_exposes("VPC-1", vec![masquerade, forwarded]), remote, remote_id: "11111".try_into().unwrap(), remote_vni: vpc3.vni, @@ -392,7 +389,6 @@ mod context { let mut vpctable = VpcTable::new(); vpctable.add(vpc1).unwrap(); - vpctable.add(vpc2).unwrap(); vpctable.add(vpc3).unwrap(); vpctable.validate().unwrap() @@ -884,7 +880,7 @@ mod std_tests { } // Ports that port forwarding has claimed on a public address are not handed out by - // masquerade, even though the two were configured by different VPCs. The claim binds the + // masquerade, even though the two were declared by separate exposes. The claim binds the // public space towards the peer, not the expose that declared it. // // The claim is on the public side. It used to be computed from the private prefixes instead, From 67f2ea57ca6b22db5f445f80389120d415ae48f0 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 16:22:14 -0600 Subject: [PATCH 19/24] test(masquerade): Union every claim in the reserved-ports oracle 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 --- nat/src/masquerade/apalloc/pool_fuzz.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 2d8512842a..6ecf9bfe01 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -748,19 +748,18 @@ fn reserved_ports_are_never_allocated() { .with_type() .cloned() .for_each(|reserved_config: ReservedConfig| { - let ranges = reserved_config.config.owner_ranges(); let pool_sets = reserved_config.pool_sets(); for (owner, allocation) in allocate_round_robin(&pool_sets, ALLOCATIONS) { let ip = allocation.ip(); let port = allocation.port().as_u16(); - // Every expose that declares this address shares the region it came from, so its - // claims apply to this allocation too. + // A claim binds the public space it names, whoever made it. The pool is built + // from the union of every claim, so the oracle unions them too: gating on the + // claimant's own ranges here would let a claim made through one expose go + // unchecked against an allocation made through another, which is the case the + // property exists to state. for (claimant, claims) in reserved_config.reservations.iter().enumerate() { - if !declares(&ranges[claimant], ip) { - continue; - } for claim in claims { assert!( !claim.covers(ip, port), From 57269054a1b1d6b7174014ea3bd121902f0e31a9 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 19:00:43 -0600 Subject: [PATCH 20/24] test(masquerade): Cover a port freed while its neighbours are still held 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/pool_fuzz.rs | 32 +++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/nat/src/masquerade/apalloc/pool_fuzz.rs b/nat/src/masquerade/apalloc/pool_fuzz.rs index 6ecf9bfe01..76dc025bc2 100644 --- a/nat/src/masquerade/apalloc/pool_fuzz.rs +++ b/nat/src/masquerade/apalloc/pool_fuzz.rs @@ -301,6 +301,38 @@ fn an_exhausted_region_falls_through_to_the_next() { ); } +/// A port given back while other ports of the same address are still held is available again. +/// +/// [`freed_allocations_become_available_again`] drops everything at once, which frees whole port +/// blocks and rebuilds them, so it passes whether or not an individual port is ever returned. A +/// flow ending while its neighbours carry on is the ordinary case, and the one that leaks: a port +/// that stays marked used is one the block cannot hand out again for as long as it lives. +#[test] +fn a_port_freed_on_its_own_is_handed_out_again() { + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE)], + idle_timeout: IDLE_TIMEOUT, + }]; + let pool_sets = + pool_sets_for_specs::(&specs, &PrefixPortsSet::new(), NextHeader::TCP, false); + + let mut held: Vec<_> = (0..5) + .map(|_| pool_sets[0].allocate(false).expect("pool has room")) + .collect(); + + // Give back one from the middle, keeping the rest, so the block stays alive. + let returned = held.remove(2); + let (ip, port) = (returned.ip(), returned.port().as_u16()); + drop(returned); + + let next = pool_sets[0].allocate(false).expect("pool has room"); + assert_eq!( + (next.ip(), next.port().as_u16()), + (ip, port), + "the port given back was not handed out again" + ); +} + /////////////////////////////////////////////////////////////////////////////// // Exhaustion /////////////////////////////////////////////////////////////////////////////// From 4027f958c6dd3d8ad6a741c7769f9dd7f1ba4150 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 21:16:08 -0600 Subject: [PATCH 21/24] test(masquerade): Hold what a racing reservation is given 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs index d199146c35..2b0b603d55 100644 --- a/nat/src/masquerade/apalloc/concurrent_fuzz.rs +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -419,16 +419,36 @@ fn packet_worker( } PacketOp::ReserveExisting => { // Race a reservation against the other threads' allocations on pools that are - // already published and in use. Failing is fine, claiming inconsistent bookkeeping - // is not. + // already published and in use. + // + // Where the writer carried this pair over, the reservation it holds is still live, + // so this one has to be refused: that is the same rule as for allocation, checked + // on the path a config change actually takes. Otherwise the pair is genuinely + // free, and a reservation that succeeds is kept for the length of the run like any + // other allocation, rather than being dropped here and handed back to the pools + // while the other threads are still working. if let Some(&(owner, ip, port)) = survivors.get(step % survivors.len().max(1)) && let Some(pool) = published.pools.get(owner) - && let Err(AllocatorError::InternalIssue(message)) = pool.reserve(ip, port) { - panic!( - "reserving {ip}:{port} in generation {}: {message}", - published.generation - ); + let carried = published.carried.contains(&(ip, port.as_u16())); + match pool.reserve(ip, port) { + Ok(reservation) => { + assert!( + !carried, + "generation {} reserved {ip}:{port} a second time, although a \ + carried-over flow already holds it", + published.generation + ); + live.claim(published.generation, ip, port.as_u16()); + held.push((published.generation, reservation)); + } + Err(AllocatorError::InternalIssue(message)) => panic!( + "reserving {ip}:{port} in generation {}: {message}", + published.generation + ), + // Refused because it is held, which is the ordinary outcome here. + Err(_) => {} + } } } } From 9654b4352e042d2c87c450eb346e51a61a78ff7e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 00:22:10 -0600 Subject: [PATCH 22/24] test(masquerade): Stop the concurrent model publishing generations that 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs index 2b0b603d55..b2dff0aa2c 100644 --- a/nat/src/masquerade/apalloc/concurrent_fuzz.rs +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -31,10 +31,16 @@ //! * An address and port is never handed to two live flows drawn from the same allocator. Shapes //! that hold every allocation for the length of the run check this exactly; those that free as //! they go trade that for exercising deallocation. See [`Live`] for why the two differ. -//! * Neither allocation nor reservation ever reports [`AllocatorError::InternalIssue`]. That is the -//! allocator saying its own bookkeeping is inconsistent, and `find_block_for_port` carries a -//! standing `FIXME` wondering whether the block it just found non-free can be released before it -//! is looked up. Reserving concurrently with allocating is what would show it. +//! * Neither allocation nor reservation ever reports [`AllocatorError::InternalIssue`]. That is +//! the allocator saying its own bookkeeping is inconsistent. +//! +//! A limit worth being honest about: `find_block_for_port` carries a standing `FIXME` wondering +//! whether the block it just found non-free can be released before it is looked up, and this +//! suite does not reach that interleaving. Reservations here 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 it would take 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, not something it does today. //! //! # No loom //! @@ -198,11 +204,14 @@ impl Published { carried.insert((ip, port.as_u16())); reservations.push(reservation); } - Err(AllocatorError::InternalIssue(message)) => { - panic!("re-reserving {ip}:{port} for generation {generation}: {message}") + // Nothing may refuse a survivor. 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. Accepting failure here would let the model + // publish generations that quietly carried nothing, and every property about + // carried pairs would pass vacuously. + Err(e) => { + panic!("re-reserving {ip}:{port} for generation {generation} failed: {e}") } - // The address may no longer be served, or another survivor may already hold it. - Err(_) => {} } } @@ -421,12 +430,13 @@ fn packet_worker( // Race a reservation against the other threads' allocations on pools that are // already published and in use. // - // Where the writer carried this pair over, the reservation it holds is still live, - // so this one has to be refused: that is the same rule as for allocation, checked - // on the path a config change actually takes. Otherwise the pair is genuinely - // free, and a reservation that succeeds is kept for the length of the run like any - // other allocation, rather than being dropped here and handed back to the pools - // while the other threads are still working. + // Every survivor is carried into every generation -- the builder panics otherwise + // -- so in a correct allocator this reservation is always refused: the writer's + // own reservation holds the pair. The refusal is the same rule as for allocation, + // checked on the path a config change actually takes, and the success arm below is + // an oracle rather than a covered path: it can only run if a bug lets a held pair + // be reserved twice, and then it must not be dropped here, or the port would go + // back to the pools mid-run and the other oracles would be reasoning over a lie. if let Some(&(owner, ip, port)) = survivors.get(step % survivors.len().max(1)) && let Some(pool) = published.pools.get(owner) { From f60f8f0de0c5765ef411a80643f9458d8844d90c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 6 Aug 2026 17:09:53 -0600 Subject: [PATCH 23/24] fix(masquerade): Answer a reservation that raced its block's release 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) Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/concurrent_fuzz.rs | 145 +++++++++++++++++- nat/src/masquerade/apalloc/port_alloc.rs | 103 +++++++++---- 2 files changed, 206 insertions(+), 42 deletions(-) diff --git a/nat/src/masquerade/apalloc/concurrent_fuzz.rs b/nat/src/masquerade/apalloc/concurrent_fuzz.rs index b2dff0aa2c..9e85a1e561 100644 --- a/nat/src/masquerade/apalloc/concurrent_fuzz.rs +++ b/nat/src/masquerade/apalloc/concurrent_fuzz.rs @@ -34,13 +34,15 @@ //! * Neither allocation nor reservation ever reports [`AllocatorError::InternalIssue`]. That is //! the allocator saying its own bookkeeping is inconsistent. //! -//! A limit worth being honest about: `find_block_for_port` carries a standing `FIXME` wondering -//! whether the block it just found non-free can be released before it is looked up, and this -//! suite does not reach that interleaving. Reservations here 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 it would take 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, not something it does today. +//! A limit worth being honest about: this suite does not reach the interleaving where the block +//! a reservation just found non-free is released before it is looked up. Reservations here +//! target survivors, and a survivor's block is pinned for the whole generation by the +//! reservation [`Published`] holds, so it cannot empty mid-lookup. That is a property of how +//! this suite reserves, not of what it takes to get there: neither a config change nor +//! generations whose specs differ is required, and +//! [`a_reservation_racing_the_release_of_its_block_is_not_called_a_bug`] reaches it with one +//! pool and one generation. Widening `ReserveExisting` to unpinned pairs would let this suite +//! reach it too, and is the extension worth making. //! //! # No loom //! @@ -74,7 +76,7 @@ use concurrency::thread; // `spawn_scoped` is inherent on std's `Builder`, but supplied by `BuilderExt` under shuttle #[cfg_attr(not(feature = "shuttle"), allow(unused_imports))] use concurrency::thread::BuilderExt; -use lpm::prefix::PrefixPortsSet; +use lpm::prefix::{PortRange, PrefixPortsSet, PrefixWithOptionalPorts}; use net::ip::NextHeader; use std::collections::BTreeSet; use std::net::Ipv4Addr; @@ -524,3 +526,130 @@ fn printing_the_pool_does_not_wedge_it_against_a_flow_ending() { releaser.join().expect("the releasing thread panicked"); }); } + +/// Reserving a port races the release of the block that holds it. +/// +/// A block is given back in two steps: its 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 can fall between the two and find the block neither free nor +/// allocated. The allocator used to call that broken bookkeeping and return `InternalIssue`, which +/// the caller reads as the allocator being unfit rather than as one flow failing. +/// +/// [`stress_test_config_change`] cannot reach this. Its reservations target survivors, and a +/// survivor's block is pinned for the whole generation by the reservation [`Published`] holds, so +/// it never empties mid-lookup. Neither a config change nor differing specs is needed to get +/// there, though -- one pool and one generation will do, which is what this pins. +/// +/// Production reaches it through the late flow `nf.rs` handles: a packet that allocated from the +/// previous allocator and installs its flow after a new one was published re-reserves its pair +/// against the current allocator, and that pair is not among the writer's pinned survivors. The +/// block behind it can be emptying at that moment. +/// +/// Either answer is legitimate -- refused while the pair is held, granted once it is released -- +/// and the test takes both. What it refuses is the allocator disclaiming its own state. +#[concurrency::model_test] +fn a_reservation_racing_the_release_of_its_block_is_not_called_a_bug() { + concurrency::stress(|| { + let specs = vec![PoolSpec { + // One address, so both threads are working on the same port allocator. + public_ranges: vec![AddrInterval::new(BASE, BASE)], + idle_timeout: IDLE_TIMEOUT, + }]; + let pools = Arc::new(pool_sets_for_specs::( + &specs, + &PrefixPortsSet::new(), + NextHeader::TCP, + false, + )); + + let allocation = pools[0].allocate(false).expect("the pool can serve"); + let (ip, port) = (allocation.ip(), allocation.port()); + + // The only holder of the pair, and so of its block: dropping it empties the block. + let releaser = thread::spawn(move || drop(allocation)); + let reserver = { + let pools = pools.clone(); + thread::spawn(move || pools[0].reserve(ip, port)) + }; + + let outcome = reserver.join().expect("the reserving thread panicked"); + releaser.join().expect("the releasing thread panicked"); + + if let Err(AllocatorError::InternalIssue(message)) = &outcome { + panic!("a reservation racing the release of its block was called a bug: {message}"); + } + }); +} + +/// Tidying a dead entry out of the list of allocated blocks races another task listing a live one. +/// +/// A task allocating from the block it used last finds its entry through a weak reference. When +/// that reference has expired the entry is dropped as it is found -- but the lookup and the drop +/// take the lock separately, so between them another task can claim the freed block and list it at +/// the same index. The drop then deletes an entry for a block that is 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 +/// find it neither free nor listed and are refused, and its free ports stop counting towards the +/// address having room. +/// +/// The third place the allocator sets a block's flag and its entry in the list apart from one +/// another, after the two in `find_block_for_port`. Here the answer is to re-check under the write +/// lock rather than to retry, since the caller has a lock to take anyway. +/// +/// Worth knowing before trusting this one: the interleaving is narrow, and `stress` gives each +/// body sixteen iterations over three schedulers. Reverting the fix is caught in roughly two runs +/// in three, not every run. It never fails with the fix, so it costs nothing in CI, but a +/// regression may take a couple of runs to show. Making the contending task poised before the +/// block is freed was tried and made it worse -- four runs in twelve rather than eight. +#[concurrency::model_test] +fn tidying_a_dead_block_entry_does_not_drop_a_live_one() { + concurrency::stress(|| { + let address = Ipv4Addr::from(u32::try_from(BASE).unwrap_or_else(|_| unreachable!())); + let claim = |lo: u16, hi: u16| { + PrefixWithOptionalPorts::new( + format!("{address}/32").as_str().into(), + Some(PortRange::new(lo, hi).unwrap_or_else(|_| unreachable!())), + ) + }; + // Two usable blocks, the first with exactly one usable port. One allocation then fills the + // first block and pins the address, which is what lets the second block die on its own -- + // dropping the last port of the only block would take the whole address with it, and the + // next allocation would build a fresh allocator with no stale entry to find. + let specs = vec![PoolSpec { + public_ranges: vec![AddrInterval::new(BASE, BASE)], + idle_timeout: IDLE_TIMEOUT, + }]; + let pools = Arc::new(pool_sets_for_specs::( + &specs, + &PrefixPortsSet::from([claim(1025, 1279), claim(1536, u16::MAX)]), + NextHeader::TCP, + false, + )); + + let _keeper = pools[0].allocate(false).expect("the keeper allocation"); + // Opens the second block, leaving this task's hint on it, and then dies: the hint now + // names an index whose entry in the list cannot be upgraded. + drop(pools[0].allocate(false).expect("the second block")); + + // One task claims the freed block and lists it... + let holder = { + let pools = pools.clone(); + thread::spawn(move || pools[0].allocate(false).ok()) + }; + // ...while this one follows its stale hint and tidies the list. + let mine = pools[0].allocate(false).ok(); + let theirs = holder.join().expect("the other task panicked"); + + // Whichever of them got it, the block is held and has free ports left, so the allocator + // has to be able to find it. + if mine.is_some() || theirs.is_some() { + let port = NatPort::new_port_checked(1400).unwrap_or_else(|_| unreachable!()); + let outcome = pools[0].reserve(address, port); + assert!( + outcome.is_ok(), + "a block in use was dropped from the list: reserving into it gave {outcome:?}" + ); + } + }); +} diff --git a/nat/src/masquerade/apalloc/port_alloc.rs b/nat/src/masquerade/apalloc/port_alloc.rs index b3ca879aed..5b57d74380 100644 --- a/nat/src/masquerade/apalloc/port_alloc.rs +++ b/nat/src/masquerade/apalloc/port_alloc.rs @@ -29,6 +29,17 @@ use rand::seq::SliceRandom; #[concurrency_mode(shuttle)] use shuttle::rand::{Rng, thread_rng}; +/// How many times a reservation will look a port's block up before giving up on it. +/// +/// A block's flag and its entry in the map of allocated blocks are set apart from one another at +/// both ends of its life, so a lookup landing between them finds it neither free nor allocated and +/// has to start over. Releasing stores the flag `true` and lets the weak entry expire with the +/// `Arc`; claiming takes the flag first and inserts the entry after building the block. Either gap +/// is enough, and the second needs no release at all -- a thread descheduled between the two holds +/// it open for as long as it is parked. This is a bound on livelock, not a number of expected +/// attempts. +const BLOCK_LOOKUP_ATTEMPTS: usize = 4; + /////////////////////////////////////////////////////////////////////////////// // AllocatorPortBlock /////////////////////////////////////////////////////////////////////////////// @@ -362,35 +373,43 @@ impl PortAllocator { ip: Arc>, port: NatPort, ) -> Result>, AllocatorError> { - let (block_was_free, index) = self.try_to_reserve_block(port)?; let allow_null = matches!(port, NatPort::Identifier(_)); - if block_was_free { - return self.allocate_block_for_reservation(ip, index, port, allow_null); - } - if let Some(block) = self.allocated_blocks.search_for_block(port) { - return Ok(block); - } - // A block masquerade may never draw from never joins the list of allocated blocks, so its - // absence from that list says nothing about the bookkeeping. Port forwarding claiming a - // block in full is the way to reach this from a configuration: a flow carried across a - // config change may hold a port in a block the new configuration has claimed. - // - // Answer as a claim on part of the same block does, where the block is allocatable and its - // own bitmap refuses the port. How much of a block an operator happened to claim is not - // something the caller should be able to tell apart, and it is certainly not the difference - // between a policy conflict and a broken allocator. - if self.block_is_excluded(port) { - debug!("Port {port} lies in a block that port forwarding has claimed in full"); - return Err(AllocatorError::PortReservationFailed(port.as_u16())); + for _ in 0..BLOCK_LOOKUP_ATTEMPTS { + let (block_was_free, index) = self.try_to_reserve_block(port)?; + if block_was_free { + return self.allocate_block_for_reservation(ip, index, port, allow_null); + } + if let Some(block) = self.allocated_blocks.search_for_block(port) { + return Ok(block); + } + // A block masquerade may never draw from never joins the list of allocated blocks, so + // its absence from that list says nothing about the bookkeeping. Port forwarding + // claiming a block in full is the way to reach this from a configuration: a flow + // carried across a config change may hold a port in a block the new configuration has + // claimed. + // + // Answer as a claim on part of the same block does, where the block is allocatable and + // its own bitmap refuses the port. How much of a block an operator happened to claim is + // not something the caller should be able to tell apart, and it is certainly not the + // difference between a policy conflict and a broken allocator. + if self.block_is_excluded(port) { + debug!("Port {port} lies in a block that port forwarding has claimed in full"); + return Err(AllocatorError::PortReservationFailed(port.as_u16())); + } + // Not free, not allocated, not excluded: the block is mid-handover. Either its last + // holder released it between the flag being read and the map being searched, or + // another thread has claimed it and not yet published it. Nothing is wrong either + // way, and starting over is the answer to both -- the next attempt finds the block + // free if it was released, and in the map once the claimant gets there. + debug!("Block holding port {port} was released mid-lookup, retrying"); } - // Block was not free but is not in the list of allocated blocks either?? - // - // FIXME: This can legitimately happen if the block was released just after we checked - // whether it was free? (Not observed in shuttle tests so far.) Do we need an additional - // lock around the PortAllocator? - Err(AllocatorError::InternalIssue( - "Block not free, although absent from list of allocated blocks".to_string(), - )) + // Every attempt landed in that window: either this block kept changing hands, or a thread + // claiming it stayed parked between taking its flag and publishing it. Say the reservation + // failed rather than claim the bookkeeping is broken -- the caller drops one flow, where + // `InternalIssue` is read as the allocator being unfit and costs the packet its whole + // batch. + debug!("Block holding port {port} was released mid-lookup {BLOCK_LOOKUP_ATTEMPTS} times"); + Err(AllocatorError::PortReservationFailed(port.as_u16())) } pub(crate) fn reserve_port( @@ -712,13 +731,27 @@ impl AllocatedPortBlockMap { self.0.read().get(&index).cloned() } - fn remove(&self, index: usize) { - self.0.write().remove(&index); + // Drop the entry at this index, but only if what is there now is still dead. + // + // The caller found its own copy of the weak reference expired, under no lock. By the time it + // gets here another thread may have claimed the freed block and inserted a live reference at + // the same index, and removing that would orphan a block that is in use: reservations into it + // would find it neither free nor listed, and its free ports would be invisible to + // `has_entries_with_free_ports`. Re-checking under the write lock is what makes the removal + // apply to the entry the caller actually saw. A different *dead* entry is fine to drop; that + // is the same tidying, one turn later. + fn remove_if_still_dead(&self, index: usize) { + let mut blocks = self.0.write(); + if let Some(stored) = blocks.get(&index) + && stored.upgrade().is_none() + { + blocks.remove(&index); + } } fn get(&self, index: usize) -> Option>> { self.get_weak(index)?.upgrade().or_else(|| { - self.remove(index); + self.remove_if_still_dead(index); None }) } @@ -735,11 +768,13 @@ impl AllocatedPortBlockMap { } fn search_for_block(&self, port: NatPort) -> Option>> { - let blocks = self.0.read(); - blocks + // One upgrade, kept. Upgrading to test the block and again to return it let the block die + // in between, so a block that was there a moment ago read as absent -- which the caller + // cannot tell from a block that was never listed. + self.0 + .read() .values() - .find(|block| block.upgrade().is_some_and(|block| block.covers(port)))? - .upgrade() + .find_map(|block| block.upgrade().filter(|block| block.covers(port))) } // Used for Display From 5a47ebc416ea34a56a9e6f83db7870032d08d1dd Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Wed, 5 Aug 2026 19:20:32 -0600 Subject: [PATCH 24/24] refactor(masquerade): Find a pool without relying on disjoint prefixes 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 Signed-off-by: Daniel Noland --- nat/src/masquerade/apalloc/mod.rs | 315 ++++++++++++++++++++++++++++-- 1 file changed, 304 insertions(+), 11 deletions(-) diff --git a/nat/src/masquerade/apalloc/mod.rs b/nat/src/masquerade/apalloc/mod.rs index fbb52cd720..4c5641c8d6 100644 --- a/nat/src/masquerade/apalloc/mod.rs +++ b/nat/src/masquerade/apalloc/mod.rs @@ -160,21 +160,46 @@ impl PoolTable { Self(BTreeMap::new()) } + /// The pool serving a private address: the entry whose prefix covers it and starts nearest to + /// it, within the same protocol and pair of VPCs. + /// + /// Keys sort by address before range end, so walking back from the address reaches the + /// prefixes that could cover it in turn. Taking only the first one found is not enough: a + /// prefix nested inside another starts nearer to an address than the prefix containing it, + /// while covering less of it, so a nested prefix would answer "no pool" for an address of the + /// wider one *above* it. The walk therefore continues past an entry that does not cover the + /// address, and stops once no later entry can be a better match. + /// + /// Where several prefixes cover the address the narrowest wins, which is the longest-prefix + /// match the rest of the system uses. Nothing in this crate rejects an overlap or reports one: + /// [`PoolTable::add_entry`] warns only when two entries have exactly the same bounds. Choosing + /// the narrowest is therefore what to do when the configuration layer's guarantee of disjoint + /// prefixes is absent, not support for a configuration it would accept. fn get(&self, key: &PoolTableKey) -> Option<&alloc::PoolSet> { - // We need to find the entry with the ID, and the prefix for the corresponding address. - // Get the range of "lower" entries, the one with the address before ours is the prefix we - // need, if the ID also matches. - match self.0.range(..=key).next_back() { - Some((k, v)) - if k.addr_range_end >= key.addr - && k.src_vpcd == key.src_vpcd - && k.dst_vpcd == key.dst_vpcd - && k.protocol == key.protocol => + let mut best: Option<(&PoolTableKey, &alloc::PoolSet)> = None; + for (candidate, pool_set) in self.0.range(..=key).rev() { + // The keys of one protocol and pair of VPCs are contiguous, so leaving that run means + // there is nothing further back to find. + if candidate.protocol != key.protocol + || candidate.src_vpcd != key.src_vpcd + || candidate.dst_vpcd != key.dst_vpcd { - Some(v) + break; + } + // Walking back, prefixes start further from the address as we go. Once one covering it + // has been found, only another starting at the same address can be narrower. + if let Some((found, _)) = best + && candidate.addr < found.addr + { + break; + } + if candidate.addr_range_end >= key.addr { + // Entries sharing a start address are visited widest first, so a later one is + // always the narrower match. + best = Some((candidate, pool_set)); } - _ => None, } + best.map(|(_, pool_set)| pool_set) } fn get_entry( @@ -474,6 +499,138 @@ fn max_range() -> I { // Tests /////////////////////////////////////////////////////////////////////////////// +#[cfg(test)] +mod bolero_tests { + use super::*; + use bolero::{Driver, TypeGenerator}; + use net::vxlan::Vni; + use std::time::Duration; + + // Prefixes are drawn from a narrow window so that nesting and overlap are the common case + // rather than astronomically unlikely, and so that every address in the window can be checked + // rather than a sampled few. + const BASE: u32 = 0x0A00_0000; // 10.0.0.0 + const WINDOW: u32 = 24; + const MAX_ENTRIES: u8 = 6; + const MAX_LEN: u8 = 12; + + // Which entry a lookup landed on, as a value a `PoolSet` can carry. + // + // Both bounds, because either alone loses entries. Marking by end made two entries ending + // together indistinguishable however far apart they start -- and "nearest start" is half of + // the rule under test, so a walk preferring the farther of the two passed. + const SPAN: u32 = WINDOW + MAX_LEN as u32 + 1; + fn marker(start: u32, end: u32) -> u32 { + (start - BASE) * SPAN + (end - start) + } + + fn vpcd(id: u32) -> VpcDiscriminant { + VpcDiscriminant::VNI(Vni::new_checked(id).unwrap()) + } + + /// A set of entries, each an offset into the window and a length, and each in one of two + /// groups so that the walk's refusal to cross between groups is exercised too. + /// + /// The foreign group sorts *before* the queried one, which is what makes that last part true: + /// keys order by destination VPC before address, so a group sorting after is cut off by + /// `range(..=key)` and never reaches the guard at all. + #[derive(Debug, Clone)] + struct Scenario { + entries: Vec<(u8, u8, bool)>, + } + + impl TypeGenerator for Scenario { + fn generate(driver: &mut D) -> Option { + let count = usize::from(driver.produce::()? % MAX_ENTRIES + 1); + let mut entries = Vec::with_capacity(count); + for _ in 0..count { + entries.push(( + driver.produce::()? % u8::try_from(WINDOW).ok()?, + driver.produce::()? % MAX_LEN + 1, + driver.produce::()?, + )); + } + Some(Self { entries }) + } + } + + impl Scenario { + // The entries of the group under test, as inclusive address bounds. + fn ranges(&self) -> Vec<(u32, u32)> { + self.entries + .iter() + .filter(|(_, _, other_group)| !other_group) + .map(|&(offset, length, _)| { + let start = BASE + u32::from(offset); + (start, start + u32::from(length) - 1) + }) + .collect() + } + + fn table(&self) -> PoolTable { + let mut table = PoolTable::new(); + for &(offset, length, other_group) in &self.entries { + let start = BASE + u32::from(offset); + let end = start + u32::from(length) - 1; + table.add_entry( + PoolTableKey::new( + NextHeader::TCP, + vpcd(1), + // Below the queried group, so the walk has to refuse to enter it. + if other_group { vpcd(2) } else { vpcd(3) }, + Ipv4Addr::from(start), + Ipv4Addr::from(end), + ), + alloc::PoolSet::new(Duration::from_secs(u64::from(marker(start, end)))), + ); + } + table + } + + // The oracle: among the entries covering the address, the one starting nearest to it, and + // of those the narrowest. Straight from the inputs, with no walking. + fn expected(&self, address: u32) -> Option { + self.ranges() + .into_iter() + .filter(|&(start, end)| start <= address && address <= end) + .min_by_key(|&(start, end)| (std::cmp::Reverse(start), end)) + .map(|(start, end)| marker(start, end)) + } + } + + /// The lookup answers what the oracle answers, on arbitrary intervals. + /// + /// Not "longest prefix", which is what this was called: the generator produces intervals of + /// any offset and length, and most of them are not CIDR-aligned. Longest-prefix match is only + /// defined on prefixes, which nest or stay disjoint; arbitrary intervals may also partially + /// overlap, and the oracle here is the rule [`PoolTable::get`] actually implements -- nearest + /// start, then narrowest -- which agrees with longest-prefix on the inputs the configuration + /// layer can produce and is defined on the ones it cannot. + #[test] + fn pool_table_lookup_matches_an_interval_oracle() { + bolero::check!() + .with_type() + .cloned() + .for_each(|scenario: Scenario| { + let table = scenario.table(); + // A margin either side, so addresses below and above every entry are covered. + for offset in 0..WINDOW + u32::from(MAX_LEN) + 2 { + let address = BASE + offset; + let found = table + .get_entry(NextHeader::TCP, vpcd(1), vpcd(3), Ipv4Addr::from(address)) + .map(|pool_set| u32::try_from(pool_set.idle_timeout().as_secs()).unwrap()); + assert_eq!( + found, + scenario.expected(address), + "lookup for {} disagreed with the oracle, entries {:?}", + Ipv4Addr::from(address), + scenario.ranges() + ); + } + }); + } +} + #[cfg(test)] mod tests { #![allow(clippy::ip_constant)] @@ -494,6 +651,142 @@ mod tests { vpcd(3) } + // A pool set carrying nothing but a distinguishable idle timeout, so a lookup can be told + // which entry it landed on. + fn marked_pool_set(marker: u64) -> alloc::PoolSet { + alloc::PoolSet::new(std::time::Duration::from_secs(marker)) + } + + // The group these tests query. Keys sort by protocol, then source VPC, then destination VPC, + // and the walk only ever looks *back* from the queried key -- so a group that sorts after this + // one is excluded by `range(..=key)` before the walk begins and proves nothing about the guard + // that stops it crossing between groups. This group is deliberately not the lowest, leaving + // room below it on each of the three components for + // [`test_the_walk_does_not_cross_into_another_group`] to put one there. + fn queried_group() -> (NextHeader, VpcDiscriminant, VpcDiscriminant) { + (NextHeader::TCP, vpcd2(), vpcd3()) + } + + fn table_with(entries: &[(&str, &str, u64)]) -> PoolTable { + let (protocol, src, dst) = queried_group(); + let mut table = PoolTable::new(); + for &(start, end, marker) in entries { + table.add_entry( + PoolTableKey::new( + protocol, + src, + dst, + start.parse().unwrap(), + end.parse().unwrap(), + ), + marked_pool_set(marker), + ); + } + table + } + + fn lookup(table: &PoolTable, addr: &str) -> Option { + let (protocol, src, dst) = queried_group(); + table + .get_entry(protocol, src, dst, addr.parse().unwrap()) + .map(|pool_set| pool_set.idle_timeout().as_secs()) + } + + // A private prefix nested inside another must not hide the addresses of the wider one. The + // lookup walks back to the nearest entry starting at or below the address, and a nested prefix + // is nearer than the prefix containing it, so stopping at the first one found answered "no + // pool" for an address the wider prefix plainly covers. A packet from it is then dropped, and + // logged as a bug in the allocator rather than as the configuration it is. + #[test] + fn test_a_nested_prefix_does_not_hide_the_one_containing_it() { + let table = table_with(&[ + ("10.0.0.0", "10.0.255.255", 16), // 10.0.0.0/16 + ("10.0.1.0", "10.0.1.255", 24), // 10.0.1.0/24, nested inside it + ]); + + // Below the nested prefix, and inside it: unambiguous either way. + assert_eq!(lookup(&table, "10.0.0.5"), Some(16)); + // Past the nested prefix, but still inside the wider one. This is the case that failed. + assert_eq!( + lookup(&table, "10.0.2.5"), + Some(16), + "an address covered by the wider prefix was not served" + ); + // Well past both. + assert_eq!(lookup(&table, "10.1.0.1"), None); + } + + // Where both cover an address, the more specific prefix serves it, as it does everywhere else + // in routing. + #[test] + fn test_the_most_specific_prefix_wins() { + let table = table_with(&[ + ("10.0.0.0", "10.0.255.255", 16), + ("10.0.1.0", "10.0.1.255", 24), + ]); + assert_eq!(lookup(&table, "10.0.1.5"), Some(24)); + } + + // Several prefixes nested one inside the next, to check the walk does not stop early. + #[test] + fn test_deeply_nested_prefixes() { + let table = table_with(&[ + ("10.0.0.0", "10.255.255.255", 8), + ("10.0.0.0", "10.0.255.255", 16), + ("10.0.0.0", "10.0.0.255", 24), + ]); + assert_eq!(lookup(&table, "10.0.0.1"), Some(24)); + assert_eq!(lookup(&table, "10.0.1.1"), Some(16)); + assert_eq!(lookup(&table, "10.1.0.1"), Some(8)); + assert_eq!(lookup(&table, "11.0.0.1"), None); + } + + // The walk stops at the edge of its own group: an entry belonging to another protocol or + // another pair of VPCs never serves an address, however well its prefix covers it. + // + // Each foreign group here sorts *before* the queried one, on a different component of the key. + // That is the whole of the test: a group sorting after is never in `range(..=key)` to begin + // with, so putting one there exercises the bound and not the guard. An earlier version of this + // test did exactly that -- the guard could be deleted outright and it still passed. + #[test] + fn test_the_walk_does_not_cross_into_another_group() { + let (protocol, src, dst) = queried_group(); + // One boundary at a time, each a group immediately below the queried one. + let foreign = [ + (NextHeader::ICMP, src, dst), + (protocol, vpcd1(), dst), + (protocol, src, vpcd2()), + ]; + + for (index, &(f_protocol, f_src, f_dst)) in foreign.iter().enumerate() { + let mut table = table_with(&[("10.0.1.0", "10.0.1.255", 24)]); + // A wider prefix, covering everything the queried group's entry does and more, but + // reached only by walking out of the queried group. + table.add_entry( + PoolTableKey::new( + f_protocol, + f_src, + f_dst, + "10.0.0.0".parse().unwrap(), + "10.0.255.255".parse().unwrap(), + ), + marked_pool_set(99), + ); + // An address only the foreign entry covers is not served at all... + assert_eq!( + lookup(&table, "10.0.2.5"), + None, + "foreign group {index} served an address of its own" + ); + // ...and one both cover is served by the queried group's entry. + assert_eq!( + lookup(&table, "10.0.1.5"), + Some(24), + "foreign group {index} displaced the entry that should serve" + ); + } + } + // Ensure that keys are sorted first by L4 protocol type, then by the source and destination // VPC IDs, and only then by IP address. This is essential to make sure we can lookup for // entries associated with prefixes for a given pair of IDs in the pool tables: the range scan