diff --git a/neqo-bin/src/udp.rs b/neqo-bin/src/udp.rs index 94488032d7..148ff43175 100644 --- a/neqo-bin/src/udp.rs +++ b/neqo-bin/src/udp.rs @@ -9,6 +9,7 @@ use std::{io, net::SocketAddr}; use neqo_common::Datagram; /// Ideally this would live in [`neqo-udp`]. [`neqo-udp`] is used in Firefox. +/// /// Firefox uses `cargo vet`. [`tokio`] the dependency of [`neqo-udp`] is not /// audited as `safe-to-deploy`. `cargo vet` will require `safe-to-deploy` for /// [`tokio`] even when behind a feature flag. diff --git a/neqo-crypto/build.rs b/neqo-crypto/build.rs index 5e8ac6fda9..34cc842b5e 100644 --- a/neqo-crypto/build.rs +++ b/neqo-crypto/build.rs @@ -66,7 +66,7 @@ fn is_debug() -> bool { // Rather than download the 400Mb+ files, like gecko does, let's just reuse their work. fn setup_clang() { // If this isn't Windows, or we're in CI, then we don't need to do anything. - if env::consts::OS != "windows" || env::var("GITHUB_WORKFLOW").unwrap() == "CI" { + if env::consts::OS != "windows" || env::var("GITHUB_WORKFLOW").unwrap_or_default() == "CI" { return; } println!("rerun-if-env-changed=LIBCLANG_PATH"); diff --git a/neqo-crypto/src/replay.rs b/neqo-crypto/src/replay.rs index 2f4613e525..dcd5775f57 100644 --- a/neqo-crypto/src/replay.rs +++ b/neqo-crypto/src/replay.rs @@ -40,6 +40,7 @@ scoped_ptr!( ); /// `AntiReplay` is used by servers when processing 0-RTT handshakes. +/// /// It limits the exposure of servers to replay attack by rejecting 0-RTT /// if it appears to be a replay. There is a false-positive rate that can be /// managed by tuning the parameters used to create the context. diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 2930af3997..4d25f38630 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1460,7 +1460,9 @@ impl Connection { ) { let space = PacketNumberSpace::from(packet.packet_type()); if let Some(space) = self.acks.get_mut(space) { - *space.ecn_marks() += d.tos().into(); + let space_ecn_marks = space.ecn_marks(); + *space_ecn_marks += d.tos().into(); + self.stats.borrow_mut().ecn_rx = *space_ecn_marks; } else { qtrace!("Not tracking ECN for dropped packet number space"); } @@ -2427,7 +2429,10 @@ impl Connection { self.loss_recovery.on_packet_sent(path, initial); } path.borrow_mut().add_sent(packets.len()); - Ok(SendOption::Yes(path.borrow_mut().datagram(packets))) + Ok(SendOption::Yes( + path.borrow_mut() + .datagram(packets, &mut self.stats.borrow_mut()), + )) } } diff --git a/neqo-transport/src/connection/tests/ecn.rs b/neqo-transport/src/connection/tests/ecn.rs index 4399ab09ff..1ac9ad6973 100644 --- a/neqo-transport/src/connection/tests/ecn.rs +++ b/neqo-transport/src/connection/tests/ecn.rs @@ -12,11 +12,11 @@ use test_fixture::{ fixture_init, now, DEFAULT_ADDR_V4, }; -use super::{send_something_with_modifier, send_with_modifier_and_receive, DEFAULT_RTT}; use crate::{ connection::tests::{ connect_force_idle, connect_force_idle_with_modifier, default_client, default_server, - handshake_with_modifier, migration::get_cid, new_client, new_server, send_something, + handshake_with_modifier, migration::get_cid, new_client, new_server, send_and_receive, + send_something, send_something_with_modifier, send_with_modifier_and_receive, DEFAULT_RTT, }, ecn::ECN_TEST_COUNT, ConnectionId, ConnectionParameters, StreamType, @@ -91,6 +91,37 @@ fn handshake_delay_with_ecn_blackhole() { ); } +#[test] +fn stats() { + let now = now(); + let mut client = default_client(); + let mut server = default_server(); + connect_force_idle(&mut client, &mut server); + + for _ in 0..ECN_TEST_COUNT { + let ack = send_and_receive(&mut client, &mut server, now); + client.process_input(&ack.unwrap(), now); + } + + for _ in 0..ECN_TEST_COUNT { + let ack = send_and_receive(&mut server, &mut client, now); + server.process_input(&ack.unwrap(), now); + } + + for stats in [client.stats(), server.stats()] { + assert_eq!(stats.ecn_paths_capable, 1); + assert_eq!(stats.ecn_paths_not_capable, 0); + + for codepoint in [IpTosEcn::Ect1, IpTosEcn::Ce] { + assert_eq!(stats.ecn_tx[codepoint], 0); + assert_eq!(stats.ecn_rx[codepoint], 0); + } + } + + assert!(client.stats().ecn_tx[IpTosEcn::Ect0] <= server.stats().ecn_rx[IpTosEcn::Ect0]); + assert!(server.stats().ecn_tx[IpTosEcn::Ect0] <= client.stats().ecn_rx[IpTosEcn::Ect0]); +} + #[test] fn disables_on_loss() { let now = now(); diff --git a/neqo-transport/src/ecn.rs b/neqo-transport/src/ecn.rs index 3329b37070..422ae5eb1e 100644 --- a/neqo-transport/src/ecn.rs +++ b/neqo-transport/src/ecn.rs @@ -12,6 +12,7 @@ use neqo_common::{qdebug, qinfo, qwarn, IpTosEcn}; use crate::{ packet::{PacketNumber, PacketType}, recovery::SentPacket, + Stats, }; /// The number of packets to use for testing a path for ECN capability. @@ -25,7 +26,7 @@ const ECN_TEST_COUNT_INITIAL_PHASE: usize = 3; /// The state information related to testing a path for ECN capability. /// See RFC9000, Appendix A.4. -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Clone, Copy)] enum EcnValidationState { /// The path is currently being tested for ECN capability, with the number of probes sent so /// far on the path during the ECN validation. @@ -50,7 +51,32 @@ impl Default for EcnValidationState { } } +impl EcnValidationState { + fn set(&mut self, new: Self, stats: &mut Stats) { + let old = std::mem::replace(self, new); + + match old { + Self::Testing { .. } | Self::Unknown => {} + Self::Failed => debug_assert!(false, "Failed is a terminal state"), + Self::Capable => stats.ecn_paths_capable -= 1, + } + match new { + Self::Testing { .. } | Self::Unknown => {} + Self::Failed => stats.ecn_paths_not_capable += 1, + Self::Capable => stats.ecn_paths_capable += 1, + } + } +} + /// The counts for different ECN marks. +/// +/// Note: [`EcnCount`] is used both for outgoing UDP datagrams, returned by +/// remote through QUIC ACKs and for incoming UDP datagrams, read from IP TOS +/// header. In the former case, given that QUIC ACKs only carry +/// [`IpTosEcn::Ect0`], [`IpTosEcn::Ect1`] and [`IpTosEcn::Ce`], but never +/// [`IpTosEcn::NotEct`], the [`IpTosEcn::NotEct`] value will always be 0. +/// +/// See also . #[derive(PartialEq, Eq, Debug, Clone, Copy, Default)] pub struct EcnCount(EnumMap); @@ -126,13 +152,13 @@ impl EcnInfo { /// Exit ECN validation if the number of packets sent exceeds `ECN_TEST_COUNT`. /// We do not implement the part of the RFC that says to exit ECN validation if the time since /// the start of ECN validation exceeds 3 * PTO, since this seems to happen much too quickly. - pub fn on_packet_sent(&mut self) { + pub fn on_packet_sent(&mut self, stats: &mut Stats) { if let EcnValidationState::Testing { probes_sent, .. } = &mut self.state { *probes_sent += 1; qdebug!("ECN probing: sent {} probes", probes_sent); if *probes_sent == ECN_TEST_COUNT { qdebug!("ECN probing concluded with {} probes sent", probes_sent); - self.state = EcnValidationState::Unknown; + self.state.set(EcnValidationState::Unknown, stats); } } } @@ -144,16 +170,17 @@ impl EcnInfo { &mut self, acked_packets: &[SentPacket], ack_ecn: Option, + stats: &mut Stats, ) -> bool { let prev_baseline = self.baseline; - self.validate_ack_ecn_and_update(acked_packets, ack_ecn); + self.validate_ack_ecn_and_update(acked_packets, ack_ecn, stats); matches!(self.state, EcnValidationState::Capable) && (self.baseline - prev_baseline)[IpTosEcn::Ce] > 0 } - pub fn on_packets_lost(&mut self, lost_packets: &[SentPacket]) { + pub fn on_packets_lost(&mut self, lost_packets: &[SentPacket], stats: &mut Stats) { if let EcnValidationState::Testing { probes_sent, initial_probes_lost: probes_lost, @@ -170,7 +197,7 @@ impl EcnInfo { "ECN validation failed, all {} initial marked packets were lost", probes_lost ); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); } } } @@ -180,6 +207,7 @@ impl EcnInfo { &mut self, acked_packets: &[SentPacket], ack_ecn: Option, + stats: &mut Stats, ) { // RFC 9000, Appendix A.4: // @@ -212,7 +240,7 @@ impl EcnInfo { // > corresponding ECN counts are not present in the ACK frame. let Some(ack_ecn) = ack_ecn else { qwarn!("ECN validation failed, no ECN counts in ACK frame"); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); return; }; @@ -229,7 +257,7 @@ impl EcnInfo { .unwrap(); if newly_acked_sent_with_ect0 == 0 { qwarn!("ECN validation failed, no ECT(0) packets were newly acked"); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); return; } let ecn_diff = ack_ecn - self.baseline; @@ -240,15 +268,16 @@ impl EcnInfo { sum_inc, newly_acked_sent_with_ect0 ); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); } else if ecn_diff[IpTosEcn::Ect1] > 0 { qwarn!("ECN validation failed, ACK counted ECT(1) marks that were never sent"); - self.state = EcnValidationState::Failed; + self.state.set(EcnValidationState::Failed, stats); } else if self.state != EcnValidationState::Capable { qinfo!("ECN validation succeeded, path is capable"); - self.state = EcnValidationState::Capable; + self.state.set(EcnValidationState::Capable, stats); } self.baseline = ack_ecn; + stats.ecn_tx = ack_ecn; self.largest_acked = largest_acked; } diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 057486e229..8c17db7930 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -453,10 +453,10 @@ impl Paths { // make a new RTT esimate and interrogate that. // That is more expensive, but it should be rare and breaking encapsulation // is worse, especially as this is only used in tests. - self.primary() - .map_or(RttEstimate::default().estimate(), |p| { - p.borrow().rtt().estimate() - }) + self.primary().map_or_else( + || RttEstimate::default().estimate(), + |p| p.borrow().rtt().estimate(), + ) } pub fn set_qlog(&mut self, qlog: NeqoQlog) { @@ -700,12 +700,12 @@ impl Path { } /// Make a datagram. - pub fn datagram>>(&mut self, payload: V) -> Datagram { + pub fn datagram>>(&mut self, payload: V, stats: &mut Stats) -> Datagram { // Make sure to use the TOS value from before calling EcnInfo::on_packet_sent, which may // update the ECN state and can hence change it - this packet should still be sent // with the current value. let tos = self.tos(); - self.ecn_info.on_packet_sent(); + self.ecn_info.on_packet_sent(stats); Datagram::new(self.local, self.remote, tos, payload) } @@ -982,7 +982,7 @@ impl Path { ) { debug_assert!(self.is_primary()); - let ecn_ce_received = self.ecn_info.on_packets_acked(acked_pkts, ack_ecn); + let ecn_ce_received = self.ecn_info.on_packets_acked(acked_pkts, ack_ecn, stats); if ecn_ce_received { let cwnd_reduced = self .sender @@ -1006,7 +1006,7 @@ impl Path { now: Instant, ) { debug_assert!(self.is_primary()); - self.ecn_info.on_packets_lost(lost_packets); + self.ecn_info.on_packets_lost(lost_packets, stats); let cwnd_reduced = self.sender.on_packets_lost( self.rtt.first_sample_time(), prev_largest_acked_sent, diff --git a/neqo-transport/src/stats.rs b/neqo-transport/src/stats.rs index e0c96b505d..ebd9c463ed 100644 --- a/neqo-transport/src/stats.rs +++ b/neqo-transport/src/stats.rs @@ -16,7 +16,7 @@ use std::{ use neqo_common::qwarn; -use crate::packet::PacketNumber; +use crate::{ecn::EcnCount, packet::PacketNumber}; pub const MAX_PTO_COUNTS: usize = 16; @@ -166,6 +166,25 @@ pub struct Stats { pub incoming_datagram_dropped: usize, pub datagram_tx: DatagramStats, + + /// Number of paths known to be ECN capable. + pub ecn_paths_capable: usize, + /// Number of paths known to be ECN incapable. + pub ecn_paths_not_capable: usize, + /// ECN counts for outgoing UDP datagrams, returned by remote through QUIC ACKs. + /// + /// Note: Given that QUIC ACKs only carry [`Ect0`], [`Ect1`] and [`Ce`], but + /// never [`NotEct`], the [`NotEct`] value will always be 0. + /// + /// See also . + /// + /// [`Ect0`]: neqo_common::tos::IpTosEcn::Ect0 + /// [`Ect1`]: neqo_common::tos::IpTosEcn::Ect1 + /// [`Ce`]: neqo_common::tos::IpTosEcn::Ce + /// [`NotEct`]: neqo_common::tos::IpTosEcn::NotEct + pub ecn_tx: EcnCount, + /// ECN counts for incoming UDP datagrams, read from IP TOS header. + pub ecn_rx: EcnCount, } impl Stats { @@ -222,7 +241,12 @@ impl Debug for Stats { writeln!(f, " frames rx:")?; self.frame_rx.fmt(f)?; writeln!(f, " frames tx:")?; - self.frame_tx.fmt(f) + self.frame_tx.fmt(f)?; + writeln!( + f, + " ecn: {:?} for tx {:?} for rx {} capable paths {} not capable paths", + self.ecn_tx, self.ecn_rx, self.ecn_paths_capable, self.ecn_paths_not_capable + ) } } diff --git a/test-fixture/src/sim/mod.rs b/test-fixture/src/sim/mod.rs index 9cf43b4b67..17e7f2b465 100644 --- a/test-fixture/src/sim/mod.rs +++ b/test-fixture/src/sim/mod.rs @@ -42,8 +42,10 @@ macro_rules! boxed { } /// Create a simulation test case. This takes either two or three arguments. +/// /// The two argument form takes a bare name (`ident`), a comma, and an array of /// items that implement `Node`. +/// /// The three argument form adds a setup block that can be used to construct a /// complex value that is then shared between all nodes. The values in the /// three-argument form have to be closures (or functions) that accept a reference