diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 2ae8a921f..3fffa33c2 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1591,6 +1591,8 @@ impl Connection { path_id: PathId, ) -> Option { let (prev_cid, prev_path) = self.paths.get_mut(&path_id)?.prev.as_mut()?; + // TODO (matheus23): We could use !prev_path.is_validating() here instead to + // (possibly) also re-send challenges when they get lost. if !prev_path.send_new_challenge { return None; }; @@ -1843,6 +1845,8 @@ impl Connection { let Some(path) = self.paths.get_mut(&path_id) else { continue; }; + self.timers + .stop(Timer::PerPath(path_id, PathTimer::PathChallengeLost)); debug!("path validation failed"); if let Some((_, prev)) = path.prev.take() { path.data = prev; @@ -1850,6 +1854,13 @@ impl Connection { path.data.challenges_sent.clear(); path.data.send_new_challenge = false; } + PathTimer::PathChallengeLost => { + let Some(path) = self.paths.get_mut(&path_id) else { + continue; + }; + trace!("path challenge deemed lost"); + path.data.send_new_challenge = true; + } PathTimer::PathOpen => { let Some(path) = self.path_mut(path_id) else { continue; @@ -4037,6 +4048,8 @@ impl Connection { } else if let Some(&challenge_sent) = path.data.challenges_sent.get(&token) { self.timers .stop(Timer::PerPath(path_id, PathTimer::PathValidation)); + self.timers + .stop(Timer::PerPath(path_id, PathTimer::PathChallengeLost)); if !path.data.validated { trace!("new path validated"); } @@ -4663,7 +4676,7 @@ impl Connection { let mut prev = mem::replace(path, new_path); // Don't clobber the original path if the previous one hasn't been validated yet - if !prev.challenges_sent.is_empty() { + if !prev.is_validating_path() { prev.send_new_challenge = true; // We haven't updated the remote CID yet, this captures the remote CID we were using on // the previous path. @@ -4925,48 +4938,48 @@ impl Connection { } // PATH_CHALLENGE - if buf.remaining_mut() > 9 && space_id == SpaceId::Data { - // Transmit challenges with every outgoing packet on an unvalidated path - if path.is_validating_path() { - // Generate a new challenge every time we send a new PATH_CHALLENGE - let token = self.rng.random(); - path.challenges_sent.insert(token, now); - sent.non_retransmits = true; - sent.requires_padding = true; - trace!("PATH_CHALLENGE {:08x}", token); - buf.write(frame::FrameType::PATH_CHALLENGE); - buf.write(token); - self.stats.frame_tx.path_challenge += 1; + if buf.remaining_mut() > 9 && space_id == SpaceId::Data && path.send_new_challenge { + path.send_new_challenge = false; - if is_multipath_negotiated && !path.validated && path.send_new_challenge { - // queue informing the path status along with the challenge - space.pending.path_status.insert(path_id); - } + // Generate a new challenge every time we send a new PATH_CHALLENGE + let token = self.rng.random(); + path.challenges_sent.insert(token, now); + sent.non_retransmits = true; + sent.requires_padding = true; + trace!("PATH_CHALLENGE {:08x}", token); + buf.write(frame::FrameType::PATH_CHALLENGE); + buf.write(token); + self.stats.frame_tx.path_challenge += 1; + let pto = self.ack_frequency.max_ack_delay_for_pto() + path.rtt.pto_base(); + self.timers.set( + Timer::PerPath(path_id, PathTimer::PathChallengeLost), + now + pto, + ); - // But only send a packet solely for that purpose at most once - path.send_new_challenge = false; + if is_multipath_negotiated && !path.validated && path.send_new_challenge { + // queue informing the path status along with the challenge + space.pending.path_status.insert(path_id); + } - // Always include an OBSERVED_ADDR frame with a PATH_CHALLENGE, regardless - // of whether one has already been sent on this path. - if space_id == SpaceId::Data - && self - .config - .address_discovery_role - .should_report(&self.peer_params.address_discovery_role) - { - let frame = - frame::ObservedAddr::new(path.remote, self.next_observed_addr_seq_no); - if buf.remaining_mut() > frame.size() { - frame.write(buf); + // Always include an OBSERVED_ADDR frame with a PATH_CHALLENGE, regardless + // of whether one has already been sent on this path. + if space_id == SpaceId::Data + && self + .config + .address_discovery_role + .should_report(&self.peer_params.address_discovery_role) + { + let frame = frame::ObservedAddr::new(path.remote, self.next_observed_addr_seq_no); + if buf.remaining_mut() > frame.size() { + frame.write(buf); - self.next_observed_addr_seq_no = - self.next_observed_addr_seq_no.saturating_add(1u8); - path.observed_addr_sent = true; + self.next_observed_addr_seq_no = + self.next_observed_addr_seq_no.saturating_add(1u8); + path.observed_addr_sent = true; - self.stats.frame_tx.observed_addr += 1; - sent.retransmits.get_or_create().observed_addr = true; - space.pending.observed_addr = false; - } + self.stats.frame_tx.observed_addr += 1; + sent.retransmits.get_or_create().observed_addr = true; + space.pending.observed_addr = false; } } } @@ -5747,6 +5760,14 @@ impl Connection { self.path_data(PathId::ZERO).current_mtu() } + /// Triggers path validation on all paths + #[cfg(test)] + pub(crate) fn trigger_path_validation(&mut self) { + for path in self.paths.values_mut() { + path.data.send_new_challenge = true; + } + } + /// Whether we have 1-RTT data to send /// /// This checks for frames that can only be sent in the data space (1-RTT): diff --git a/quinn-proto/src/connection/timer.rs b/quinn-proto/src/connection/timer.rs index c7e68474c..f54f79dba 100644 --- a/quinn-proto/src/connection/timer.rs +++ b/quinn-proto/src/connection/timer.rs @@ -44,25 +44,28 @@ pub(crate) enum PathTimer { PathIdle = 1, /// When to give up on validating a new path from RFC9000 migration PathValidation = 2, + /// When to resend a path challenge deemed lost + PathChallengeLost = 3, /// When to give up on validating a new (multi)path - PathOpen = 3, + PathOpen = 4, /// When to send a `PING` frame to keep the path alive - PathKeepAlive = 4, + PathKeepAlive = 5, /// When pacing will allow us to send a packet - Pacing = 5, + Pacing = 6, /// When to send an immediate ACK if there are unacked ack-eliciting packets of the peer - MaxAckDelay = 6, + MaxAckDelay = 7, /// When to clean up state for an abandoned path - PathAbandoned = 7, + PathAbandoned = 8, /// When the peer fails to confirm abandoning the path - PathNotAbandoned = 8, + PathNotAbandoned = 9, } impl PathTimer { - const VALUES: [Self; 9] = [ + const VALUES: [Self; 10] = [ Self::LossDetection, Self::PathIdle, Self::PathValidation, + Self::PathChallengeLost, Self::PathOpen, Self::PathKeepAlive, Self::Pacing, diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index e941476a7..c9fe23c9a 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -1346,43 +1346,62 @@ fn path_challenge_retransmit() { let (client_ch, server_ch) = pair.connect(); pair.drive(); - let challenges_sent_before = pair - .server_conn_mut(server_ch) - .stats() - .frame_tx - .path_challenge; + pair.client_conn_mut(client_ch).ping(); + pair.drive(); - println!("-------- client migrates --------"); - pair.client.addr = SocketAddr::new( - Ipv4Addr::new(127, 0, 0, 1).into(), - CLIENT_PORTS.lock().unwrap().next().unwrap(), - ); - // Send more than a ping to make sure we have enough anti-amplification budget to resend - let stream_id = pair.client_streams(client_ch).open(Dir::Uni).unwrap(); - let to_write = [0u8; 1000]; - let mut written = 0; - while written < 1000 { - written += pair - .client_conn_mut(client_ch) - .send_stream(stream_id) - .write(&to_write[written..]) - .unwrap(); - } - - pair.drive_client(); // This will send the stream datagram - pair.drive_server(); // This will make the server receive the stream datagram, increase its anti-amp budget, and send the first path challenge + println!("-------- server wants path validation --------"); + pair.server_conn_mut(server_ch).trigger_path_validation(); + pair.drive_server(); // Send the path challenge println!("-------- client loses messages --------"); // Have the client lose the challenge pair.client.inbound.clear(); pair.drive(); + let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx; + let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx; + assert_eq!( - pair.server_conn_mut(server_ch) - .stats() - .frame_tx - .path_challenge, - challenges_sent_before + 2 + server_tx.path_challenge, 2, + "expected server to send two path challenges" + ); + assert_eq!( + client_tx.path_response, 1, + "expected client to send one path response" + ); +} + +#[test] +fn path_response_retransmit() { + let _guard = subscribe(); + let mut pair = Pair::default(); + let (client_ch, server_ch) = pair.connect(); + pair.drive(); + + pair.client_conn_mut(client_ch).ping(); + pair.drive(); + + println!("-------- server wants path validation --------"); + pair.server_conn_mut(server_ch).trigger_path_validation(); + pair.drive_server(); // Send the path challenge + pair.drive_client(); // Send the path response + println!("-------- server loses messages --------"); + // Have the server lose the path response + pair.server.inbound.clear(); + + // The server should decide to re-send the path challenge + pair.drive(); + + let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx; + let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx; + + assert_eq!( + server_tx.path_challenge, 2, + "expected server to send two path challenges" + ); + assert_eq!( + client_tx.path_response, 2, + "expected client to send two path responses" ); }