mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-09 00:04:54 +00:00
Mirror reactive MtuExceeded into path_mtu_lookup
When a transit forwarder drops an oversized data packet and reports the bottleneck back via MtuExceeded, the receive-side handler already updates per-session MmpSessionState::path_mtu (used by PTB synthesis to feed kernel TCP). It did not, however, update path_mtu_lookup — the per-destination map the TUN reader/writer consult at TCP MSS clamp time. So forward-path-asymmetry flows kept clamping at the discovery reverse-path value (too generous for the actual forward-path budget) on every subsequent SYN. Add the missing write at the same point apply_notification runs. Keep the tighter of existing-or-new — the clamp must never loosen. Same write-shape as seed_path_mtu_for_link_peer. Tests: - Three focused unit tests on handle_mtu_exceeded for the empty, tighten, and keep-tighter cases. - Extended test_multihop_pmtud_heterogeneous_mtu to assert the lookup tightens after the wire-level MtuExceeded propagation, alongside its existing PathMtuState assertion. Adds two #[cfg(test)] accessors on Node (path_mtu_lookup_get / path_mtu_lookup_insert) and bumps handle_mtu_exceeded to pub(in crate::node) for direct test invocation.
This commit is contained in:
@@ -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) ===
|
||||
|
||||
@@ -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<u16> {
|
||||
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()
|
||||
|
||||
@@ -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<u8> {
|
||||
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"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user