diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 9fca0e5..0f9a795 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -27,7 +27,7 @@ use crate::protocol::{ use crate::protocol::{coords_wire_size, encode_coords}; use crate::upper::icmp::FIPS_OVERHEAD; use secp256k1::PublicKey; -use tracing::{debug, info, trace}; +use tracing::{debug, info, trace, warn}; impl Node { /// Handle a locally-delivered session datagram payload. @@ -1119,7 +1119,7 @@ impl Node { /// A transit router couldn't forward our packet because it exceeded the /// next-hop transport MTU. Apply the reported bottleneck MTU to our /// PathMtuState for the affected session, causing an immediate decrease. - async fn handle_mtu_exceeded(&mut self, inner: &[u8]) { + pub(in crate::node) async fn handle_mtu_exceeded(&mut self, inner: &[u8]) { self.stats_mut().errors.mtu_exceeded += 1; let msg = match MtuExceeded::decode(inner) { @@ -1155,6 +1155,47 @@ impl Node { ); } } + + // Mirror the bottleneck into the FipsAddress-keyed lookup used by + // the TUN reader/writer at TCP MSS clamp time. Discovery's reverse- + // path response can carry a value too generous for the actual + // forward path; the reactive signal from a forwarder that actually + // dropped a packet is authoritative for "what fits". Keep the + // tighter of existing-or-new — never loosen the clamp. + let fips_addr = crate::FipsAddress::from_node_addr(&msg.dest_addr); + match self.path_mtu_lookup.write() { + Ok(mut map) => match map.get(&fips_addr).copied() { + Some(existing) if existing <= msg.mtu => { + debug!( + dest = %peer_name, + fips_addr = %fips_addr, + bottleneck_mtu = msg.mtu, + existing, + "Reactive MtuExceeded: keeping tighter existing path_mtu_lookup value" + ); + } + other => { + map.insert(fips_addr, msg.mtu); + debug!( + dest = %peer_name, + fips_addr = %fips_addr, + bottleneck_mtu = msg.mtu, + prior = ?other, + map_len = map.len(), + "Reactive MtuExceeded: tightened path_mtu_lookup" + ); + } + }, + Err(e) => { + warn!( + dest = %peer_name, + fips_addr = %fips_addr, + bottleneck_mtu = msg.mtu, + error = %e, + "path_mtu_lookup write lock poisoned; reactive MtuExceeded not reflected" + ); + } + } } // === Session Initiation (Send Path) === diff --git a/src/node/mod.rs b/src/node/mod.rs index 240690d..3d95d47 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1581,6 +1581,23 @@ impl Node { self.sessions.remove(remote) } + /// Read the path_mtu_lookup entry for a destination FipsAddress. + #[cfg(test)] + pub(crate) fn path_mtu_lookup_get(&self, fips_addr: &crate::FipsAddress) -> Option { + self.path_mtu_lookup + .read() + .ok() + .and_then(|map| map.get(fips_addr).copied()) + } + + /// Write a path_mtu_lookup entry directly (for tests that pre-seed the map). + #[cfg(test)] + pub(crate) fn path_mtu_lookup_insert(&self, fips_addr: crate::FipsAddress, mtu: u16) { + if let Ok(mut map) = self.path_mtu_lookup.write() { + map.insert(fips_addr, mtu); + } + } + /// Number of end-to-end sessions. pub fn session_count(&self) -> usize { self.sessions.len() diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index d56e3f5..4897d40 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -2031,6 +2031,20 @@ async fn test_multihop_pmtud_heterogeneous_mtu() { path_mtu ); + // Verify path_mtu_lookup (consulted by the TUN reader/writer at TCP MSS + // clamp time) also reflects the tightened bottleneck. The reactive + // MtuExceeded handler writes here so subsequent SYN clamps see the + // forward-path budget rather than the discovery reverse-path value. + let lookup_mtu = nodes[0] + .node + .path_mtu_lookup_get(&dst_fips) + .expect("path_mtu_lookup should have entry for C after MtuExceeded"); + assert!( + lookup_mtu < 1400, + "path_mtu_lookup should have tightened from MtuExceeded signal, got {}", + lookup_mtu + ); + // Now send ANOTHER oversized packet — this time handle_tun_outbound // should check PathMtuState and generate ICMPv6 PTB on TUN instead // of forwarding. @@ -2096,3 +2110,100 @@ async fn test_multihop_pmtud_heterogeneous_mtu() { cleanup_nodes(&mut nodes).await; } + +// ============================================================================ +// Reactive MtuExceeded → path_mtu_lookup focused unit tests +// +// These exercise the receive-side write path that mirrors the bottleneck +// MTU into `path_mtu_lookup` (consulted by the TUN reader/writer at +// SYN-clamp time). Discovery's reverse-path response and the FMP-promotion +// seed populate the same lookup; the reactive channel keeps it +// authoritative under forward-path-asymmetry conditions. +// ============================================================================ + +/// Build an MtuExceeded inner payload (35 bytes: flags + dest + reporter + mtu LE). +/// +/// `handle_mtu_exceeded` receives the payload after the dispatcher strips +/// the FSP prefix and msg_type byte, so the test wire is just the body. +fn build_mtu_exceeded_inner(dest: &NodeAddr, reporter: &NodeAddr, mtu: u16) -> Vec { + let mut buf = Vec::with_capacity(35); + buf.push(0x00); // flags (reserved) + buf.extend_from_slice(dest.as_bytes()); + buf.extend_from_slice(reporter.as_bytes()); + buf.extend_from_slice(&mtu.to_le_bytes()); + buf +} + +#[tokio::test] +async fn test_handle_mtu_exceeded_writes_path_mtu_lookup_when_empty() { + use crate::node::tests::spanning_tree::make_test_node; + + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCC; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + assert!( + tn.node.path_mtu_lookup_get(&dest_fips).is_none(), + "lookup should start empty for this destination" + ); + + let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.path_mtu_lookup_get(&dest_fips), + Some(1280), + "MtuExceeded should populate path_mtu_lookup with the bottleneck MTU" + ); +} + +#[tokio::test] +async fn test_handle_mtu_exceeded_tightens_existing_path_mtu_lookup() { + use crate::node::tests::spanning_tree::make_test_node; + + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCC; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + // Pre-seed with a generous value (e.g., from a discovery reverse-path + // response that didn't reflect the forward-path bottleneck). + tn.node.path_mtu_lookup_insert(dest_fips, 1500); + + let inner = build_mtu_exceeded_inner(&dest, &reporter, 1280); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.path_mtu_lookup_get(&dest_fips), + Some(1280), + "MtuExceeded with smaller bottleneck must tighten the lookup" + ); +} + +#[tokio::test] +async fn test_handle_mtu_exceeded_keeps_tighter_existing_path_mtu_lookup() { + use crate::node::tests::spanning_tree::make_test_node; + + let mut tn = make_test_node().await; + + let dest = NodeAddr::from_bytes([0xCC; 16]); + let reporter = NodeAddr::from_bytes([0xBB; 16]); + let dest_fips = crate::FipsAddress::from_node_addr(&dest); + + // Pre-seed with a tighter value than the incoming signal (e.g., from + // a prior reactive event on a narrower hop). The clamp must never + // loosen — keep the existing value. + tn.node.path_mtu_lookup_insert(dest_fips, 1280); + + let inner = build_mtu_exceeded_inner(&dest, &reporter, 1500); + tn.node.handle_mtu_exceeded(&inner).await; + + assert_eq!( + tn.node.path_mtu_lookup_get(&dest_fips), + Some(1280), + "MtuExceeded with looser bottleneck must not loosen a tighter existing value" + ); +}