diff --git a/CHANGELOG.md b/CHANGELOG.md index cd80dcee..2ae336ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -594,6 +594,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 "already on this exact path and it is fresh". `connect` stays ephemeral: the peer is not written to configuration and gets no auto-reconnect. +- A path MTU measured on one link no longer clamps a peer that has moved to + another. Every writer of the per-destination path-MTU cache keeps the + smaller of the existing and incoming value, which is right while a peer + stays put, but the entry is keyed by destination alone. So a peer first + reached over a narrow link stayed clamped to that link's ceiling for the + lifetime of the process: when it later became reachable over a wider + transport, promotion re-seeded, the seed saw a tighter existing value and + declined, and traffic kept running at the old link's ceiling on a link that + could carry far more, with nothing reporting it because the clamp was doing + exactly what it was told. The node now records which transport last seeded + each destination and treats a seed from a different one as authoritative + rather than as a loosening to refuse. Re-seeding the same transport still + keeps the tighter value, so repeated promotion does not reset discovery, and + a destination with no prior seed is unchanged. + ### Security - The influence a remote party has over path MTU is now bounded, and the diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 9685db00..66fe5f21 100644 --- a/src/node/handlers/lookup.rs +++ b/src/node/handlers/lookup.rs @@ -735,6 +735,19 @@ impl Node { /// `path_mtu_lookup` empty for their FipsAddress, causing /// `per_flow_max_mss` to fall back to the global ceiling and the /// SYN-time TCP MSS clamp to over-estimate the effective path. + /// + /// The never-loosen rule is scoped to a single link. A tighter value is + /// evidence about the path it was measured on, so re-seeding from a + /// *different* transport than the one that last seeded this destination + /// replaces it outright: the peer has moved, and the old measurement + /// describes a path it no longer uses. Without that, a peer once + /// reachable only over a low-MTU link stays clamped to it for the process + /// lifetime even after moving to a wider one. + /// + /// A destination with no prior seed keeps the never-loosen rule unchanged + /// — nothing yet says which link its value describes, so a value learned + /// from discovery or from reactive `MtuExceeded` is assumed to be about + /// the link now being seeded and is not discarded. pub(in crate::node) fn seed_path_mtu_for_link_peer( &self, peer_addr: &NodeAddr, @@ -774,9 +787,33 @@ impl Node { ); return; }; + // Taken while `path_mtu_lookup` is held. This is the only site that + // locks both, so no lock-order inversion is reachable. + let Ok(mut seeded_by) = self.path_mtu_seeded_by.write() else { + warn!( + peer = %self.peer_display_name(peer_addr), + "seed_path_mtu_for_link_peer: path_mtu_seeded_by write lock poisoned" + ); + return; + }; + // Only a *prior seed from another transport* proves the peer has + // moved. With no prior seed the existing value came from discovery or + // reactive learning about the path we are seeding now, so the + // never-loosen rule still applies to it. + let prior_seed = seeded_by.get(&fips_addr).copied(); + let relinked = prior_seed.is_some_and(|prior| prior != transport_id); + // Recorded whether or not the value changes: the next seed needs to + // know which link this one described, otherwise a peer whose first + // seed was declined never registers a link at all and a later move + // cannot be detected. + seeded_by.insert(fips_addr, transport_id); match map.get(&fips_addr).copied() { - Some(existing) if existing.mtu <= link_mtu => { - // Keep the tighter learned value; never loosen the clamp. + Some(existing) if !relinked && existing.mtu <= link_mtu => { + // Keep the tighter learned value; never loosen within a link. + // `relinked` is the case upstream's held/release lifecycle does + // not reach: two links to one peer can be up at once, so the + // old entry is never released and a wider seed from the new + // transport would otherwise be refused forever. debug!( peer = %self.peer_display_name(peer_addr), fips_addr = %fips_addr, @@ -794,6 +831,9 @@ impl Node { fips_addr = %fips_addr, link_mtu = link_mtu, prior = ?other, + prior_transport = ?prior_seed, + transport_id = %transport_id, + relinked = relinked, map_len = map.len(), "seed_path_mtu_for_link_peer: wrote link MTU" ); diff --git a/src/node/mod.rs b/src/node/mod.rs index 7a78aa0b..5d22f067 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -339,6 +339,18 @@ pub struct Node { /// SYN/SYN-ACK clamp can use the smaller of the local-egress floor /// and the learned per-destination path MTU. path_mtu_lookup: crate::upper::tun::PathMtuLookup, + /// Which transport last supplied a *link seed* into `path_mtu_lookup`, + /// per destination. + /// + /// A `PathMtuEntry` is released when the link that seeded it goes away, + /// but two links to one peer can be up at the same time — a phone on both + /// BLE and Wi-Fi Aware, say. Then nothing releases the first entry and a + /// wider seed from the second transport is refused by the never-loosen + /// rule forever. Recording the seeding transport is what distinguishes a + /// value that still describes the current path from one that describes a + /// path the peer has left. Absent for destinations reached over multiple + /// hops: those are never link-seeded, so never-loosen applies unchanged. + path_mtu_seeded_by: Arc>>, // === Transports & Links === /// Active transports (owned by Node). @@ -753,6 +765,7 @@ impl Node { peer_acl, host_map, path_mtu_lookup: Arc::new(std::sync::RwLock::new(HashMap::new())), + path_mtu_seeded_by: Arc::new(std::sync::RwLock::new(HashMap::new())), #[cfg(unix)] decrypt_registered_sessions: std::collections::HashSet::new(), #[cfg(unix)] @@ -896,6 +909,7 @@ impl Node { peer_acl, host_map, path_mtu_lookup: Arc::new(std::sync::RwLock::new(HashMap::new())), + path_mtu_seeded_by: Arc::new(std::sync::RwLock::new(HashMap::new())), #[cfg(unix)] decrypt_registered_sessions: std::collections::HashSet::new(), #[cfg(unix)] diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 256f820e..1d1f5cf9 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -1942,6 +1942,142 @@ async fn test_seed_path_mtu_noop_for_unknown_transport() { ); } +/// The upgrade case, and the reason the seeding transport is tracked. +/// +/// A peer first reachable only over a narrow link, then moving to a wider +/// one, must not stay clamped to the narrow link's MTU. Every writer of +/// `path_mtu_lookup` keeps the tighter value, so without recording which link +/// a value described, the low MTU outlives the link it came from and pins the +/// peer for the process lifetime. +#[tokio::test] +async fn test_seed_path_mtu_reseeds_when_peer_moves_to_wider_transport() { + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.supervisor.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let narrow = make_udp_transport_with_mtu(1, 1280).await; + let wide = make_udp_transport_with_mtu(2, 1452).await; + node.transports.insert(TransportId::new(1), narrow); + node.transports.insert(TransportId::new(2), wide); + + let peer_addr = make_node_addr(0xE1); + let fips_addr = crate::FipsAddress::from_node_addr(&peer_addr); + let narrow_addr = TransportAddr::from_string("10.0.0.6:2121"); + let wide_addr = TransportAddr::from_string("10.0.0.7:2121"); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &narrow_addr); + assert_eq!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .map(|e| e.mtu), + Some(1280), + "first seed takes the narrow link's MTU" + ); + + // The peer moves to the wider transport. + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(2), &wide_addr); + assert_eq!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .map(|e| e.mtu), + Some(1452), + "a seed from a different transport must replace a value describing \ + the link the peer has left" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + +/// A value learned *about the narrow link* is discarded on the move too — it +/// measured a path the peer no longer uses. +#[tokio::test] +async fn test_seed_path_mtu_discards_learned_value_from_abandoned_link() { + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.supervisor.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let narrow = make_udp_transport_with_mtu(1, 1280).await; + let wide = make_udp_transport_with_mtu(2, 1452).await; + node.transports.insert(TransportId::new(1), narrow); + node.transports.insert(TransportId::new(2), wide); + + let peer_addr = make_node_addr(0xE2); + let fips_addr = crate::FipsAddress::from_node_addr(&peer_addr); + let narrow_addr = TransportAddr::from_string("10.0.0.8:2121"); + let wide_addr = TransportAddr::from_string("10.0.0.9:2121"); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &narrow_addr); + // Reactive MtuExceeded tightens further, still on the narrow link. + node.path_mtu_lookup + .write() + .unwrap() + .insert(fips_addr, crate::upper::tun::PathMtuEntry::held(900)); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(2), &wide_addr); + assert_eq!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .map(|e| e.mtu), + Some(1452), + "a tighter value measured on the abandoned link must not clamp the new one" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + +/// The guard against over-loosening. Promotion re-seeds on every handshake, +/// so discarding a tighter learned value on a *same-link* re-seed would reset +/// genuine PMTU discovery repeatedly and the estimate would never converge. +#[tokio::test] +async fn test_seed_path_mtu_keeps_tighter_value_when_reseeding_same_transport() { + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.supervisor.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let udp = make_udp_transport_with_mtu(1, 1452).await; + node.transports.insert(TransportId::new(1), udp); + + let peer_addr = make_node_addr(0xE3); + let fips_addr = crate::FipsAddress::from_node_addr(&peer_addr); + let transport_addr = TransportAddr::from_string("10.0.0.10:2121"); + + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &transport_addr); + // Reactive learning tightens the same link. + node.path_mtu_lookup + .write() + .unwrap() + .insert(fips_addr, crate::upper::tun::PathMtuEntry::held(1200)); + + // Re-promotion on the same transport. + node.seed_path_mtu_for_link_peer(&peer_addr, TransportId::new(1), &transport_addr); + assert_eq!( + node.path_mtu_lookup + .read() + .unwrap() + .get(&fips_addr) + .map(|e| e.mtu), + Some(1200), + "re-seeding the same link must not undo reactive learning" + ); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + // === Outbound admission gate tests === /// Inject `count` synthetic active peers into `node.peers` so peer_count()