mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-06 03:28:24 +00:00
fix(node): scope the path-MTU never-loosen rule to the link it measured
`path_mtu_lookup` is only ever tightened — every writer keeps the smaller
of the existing and incoming value. That is correct while a peer stays on
one link, but the entry is keyed by destination alone, so a value keeps
applying after the peer has moved to a different transport with a
different MTU.
The result is a one-way ratchet. A peer first reached over a narrow link
is clamped to that link's MTU for the lifetime of the process: when it
later becomes reachable over a wider transport, promotion re-seeds,
`seed_path_mtu_for_link_peer` sees a tighter existing value, and declines.
Traffic keeps running at the narrow link's ceiling on a link that could
carry far more, and nothing reports it — the clamp is doing exactly what
it was told.
Record which transport last seeded each destination, and treat a seed from
a *different* transport as authoritative rather than as a loosening to be
refused: the stored value measured a path the peer no longer uses, whether
it came from the earlier link seed or from discovery or reactive
`MtuExceeded` learning on top of it.
Two cases deliberately keep the old behaviour:
- Re-seeding the *same* transport still keeps the tighter value.
Promotion re-seeds on every handshake, so discarding learned values on
a same-link re-seed would reset PMTU discovery repeatedly and the
estimate would never converge.
- A destination with no prior seed still keeps the tighter value.
Nothing yet says which link its value describes, so a value learned
from discovery is assumed to describe the link now being seeded. The
seeding transport is recorded even when the value is declined, so a
later move is still detected.
The tracking map is a sibling rather than a widened value type, because
`PathMtuLookup` is public and read directly by the TUN clamp threads;
readers are unchanged. It is written only in `seed_path_mtu_for_link_peer`,
the only site that holds both locks, so no lock-order inversion is
reachable.
Adds three tests: the move to a wider transport, the same move with a
tighter value learned on the abandoned link, and the same-link re-seed
that must not undo reactive learning.
--- changelog ---
One entry under Fixed, describing the one-way ratchet, the transport
that is now recorded against each destination, and the two cases that
deliberately keep the old behaviour.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
|
||||
@@ -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<std::sync::RwLock<HashMap<crate::FipsAddress, TransportId>>>,
|
||||
|
||||
// === 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)]
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user