mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-12 09:33:23 +00:00
Make Node::transport_mtu() deterministic across restarts (TCP black hole fix)
Default-config TCP flows between fips peers were stalling completely (cwnd-pinned, 0 bps for 10s+) on a non-trivial fraction of restarts. Reproducible with iperf3 between any two peers. Root cause: `Node::transport_mtu()` iterated `self.transports.values()` (HashMap with default RandomState hasher) and returned `handle.mtu()` of the first one whose `is_operational()` returned true. Two stacked sources of non-determinism stacked on each other: HashMap iteration order is randomized per-process via RandomState, and async transport `.start()` completion order races each daemon restart. The returned value drives the TCP MSS clamp ceiling computed once at TUN init (src/upper/tun.rs:501-524) and stored as immutable max_mss in the reader/writer thread state. When the picker landed on a transport with MTU > 1357 (any non-UDP-1280 in the standard fleet defaults), `max_mss > 1220` (kernel's natural fips0-MTU-derived MSS), the daemon's clamp was silently a no-op, and the kernel emitted 1220-byte segments. Those wrap into 1280-byte IPv6 → 1357-byte fips datagrams that exceed UDP-1280 transports at any forwarding hop, causing silent drops with no PTB feedback to the kernel TCP stack. Fix: return min across operational transports instead of first-iterated. With UDP-1280 in the configured set (the common case), `transport_mtu = 1280`, `max_mss = 1143 < 1220`, daemon's clamp engages, MSS=1143 reaches the wire, packets fit, throughput recovers. Empirical green light from a single-UDP-config end-to-end test: iperf3-without-`-M` recovered to ~21 Mbps with no operator-side nft TCPMSS rules. Adds three unit tests: - transport_mtu_returns_min_across_operational: pin selection to smallest MTU when multiple operational transports differ. - transport_mtu_fallback_when_no_operational_transports: 1280 fallback. - transport_mtu_min_with_single_operational: trivial single-transport case. The `effective_ipv6_mtu` field reported by `fipsctl show status` was also racy (consequence of the same bug); fixed by this change as a side effect.
This commit is contained in:
@@ -951,3 +951,82 @@ fn test_promote_clears_retry_pending() {
|
||||
"retry_pending should be cleared on successful promotion"
|
||||
);
|
||||
}
|
||||
|
||||
// ============================================================================
|
||||
// transport_mtu() — ISSUE-2026-0011 regression coverage
|
||||
// ============================================================================
|
||||
|
||||
/// Helper: spawn a UdpTransport with the given mtu, started and operational.
|
||||
async fn make_udp_transport_with_mtu(id: u32, mtu: u16) -> TransportHandle {
|
||||
let (packet_tx, _packet_rx) = packet_channel(64);
|
||||
let transport_id = TransportId::new(id);
|
||||
let mut udp = UdpTransport::new(
|
||||
transport_id,
|
||||
Some(format!("udp{}", id)),
|
||||
crate::config::UdpConfig {
|
||||
bind_addr: Some("127.0.0.1:0".to_string()),
|
||||
mtu: Some(mtu),
|
||||
..Default::default()
|
||||
},
|
||||
packet_tx,
|
||||
);
|
||||
udp.start_async().await.unwrap();
|
||||
TransportHandle::Udp(udp)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_transport_mtu_returns_min_across_operational() {
|
||||
// Multiple operational transports with varied MTUs. The picker must
|
||||
// return the smallest, deterministically, regardless of HashMap
|
||||
// iteration order. This is the core ISSUE-2026-0011 regression test.
|
||||
let mut node = make_node();
|
||||
let (packet_tx, packet_rx) = packet_channel(64);
|
||||
node.packet_tx = Some(packet_tx);
|
||||
node.packet_rx = Some(packet_rx);
|
||||
|
||||
let udp1 = make_udp_transport_with_mtu(1, 1497).await;
|
||||
let udp2 = make_udp_transport_with_mtu(2, 1280).await;
|
||||
let udp3 = make_udp_transport_with_mtu(3, 1400).await;
|
||||
|
||||
node.transports.insert(TransportId::new(1), udp1);
|
||||
node.transports.insert(TransportId::new(2), udp2);
|
||||
node.transports.insert(TransportId::new(3), udp3);
|
||||
|
||||
// Expect the smallest (UDP-1280), not whichever HashMap iterates first.
|
||||
assert_eq!(node.transport_mtu(), 1280);
|
||||
|
||||
// effective_ipv6_mtu = 1280 - 77 = 1203, max_mss = 1203 - 60 = 1143
|
||||
// (verifies the downstream clamp value).
|
||||
assert_eq!(node.effective_ipv6_mtu(), 1203);
|
||||
|
||||
for transport in node.transports.values_mut() {
|
||||
transport.stop().await.ok();
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_transport_mtu_fallback_when_no_operational_transports() {
|
||||
// No transports configured at all → falls back to 1280 (IPv6 minimum).
|
||||
let node = make_node();
|
||||
assert_eq!(node.transport_mtu(), 1280);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_transport_mtu_min_with_single_operational() {
|
||||
// Single transport: trivially returns its MTU. Pins the picker doesn't
|
||||
// accidentally drop down to a smaller fallback when one transport is
|
||||
// operational.
|
||||
let mut node = make_node();
|
||||
let (packet_tx, packet_rx) = packet_channel(64);
|
||||
node.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);
|
||||
|
||||
assert_eq!(node.transport_mtu(), 1452);
|
||||
|
||||
for transport in node.transports.values_mut() {
|
||||
transport.stop().await.ok();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user