diff --git a/crates/irtt-client/AGENTS.md b/crates/irtt-client/AGENTS.md index f025376..f72ba5a 100644 --- a/crates/irtt-client/AGENTS.md +++ b/crates/irtt-client/AGENTS.md @@ -26,30 +26,30 @@ configuration (`try_enable_tx_timestamping` restores it explicitly) and timing falls back to userspace `sent_at` as it always has. TX capability failure is never a public error and is never logged from the library. -### Correlation invariant: kernel ID == wire sequence +### Best-effort kernel-ID correlation -The client uses the kernel's automatic `OPT_ID` directly as the probe's wire -sequence number — no separate correlation map. This is safe because: +The client uses the kernel's automatic `OPT_ID` as the probe's wire-sequence +correlation ID — no separate correlation map. On the normal successful-send +path, both counters start at zero after Open and advance together. This is +not a permanent identity: a kernel ID can theoretically be consumed while a +send is being prepared and then survive a subsequent send failure, leaving +kernel IDs one or more steps ahead of `next_wire_seq`. Do not restore an +assumption that failed or nonblocking sends cannot cause that desynchrony. -- **A failed/nonblocking send never consumes an ID.** Linux increments the - per-socket ID counter (`sk->sk_tskey`) speculatively while building a - datagram and rolls the increment back (`atomic_dec`) on every error path - out of `__ip_append_data`/`__ip6_append_data` - (`net/ipv4/ip_output.c`, `net/ipv6/ip6_output.c`), guarded by a local - `hold_tskey` flag. A `WouldBlock` `try_send` therefore can never leave a - gap in, or otherwise advance, the ID the kernel will assign to the next - *successful* submission. (`include/net/sock.h`'s `_sock_tx_timestamp` - performs the same increment for the non-corked path.) Client-side, the - existing send transaction only calls `commit_probe_sent` — which is the - only thing that advances `next_wire_seq` — after a successful socket send; - a `WouldBlock` result never reaches it. -- **The two counters start together and advance together.** `wire_seq` - starts at 0 on every successful Open (`SessionMachine::commit_open` builds - a fresh `ActiveSession` with `next_wire_seq: 0`) and advances by exactly 1 - only via `commit_probe_sent`, which only runs after a confirmed successful - send — the same condition under which the kernel's counter advances by 1. - TX timestamping is enabled only after that same successful Open and before - any probe is sent, so both counters' zero points line up exactly. +The effect is confined to the optional timing enhancement. A kernel timestamp +whose ID cannot be matched to a pending or retained timed-out probe is +discarded, as is any implausible timestamp; `sent_at` remains the ordinary +userspace fallback. **After every failed probe submission, disable kernel-ID +correlation for the rest of that session.** A gap means an otherwise plausible +later ID can name the wrong probe, so subsequent error-queue timestamps must +be discarded rather than attached by ID. This is safe because: + +- **Normal successful sends track together.** `wire_seq` starts at 0 on every + successful Open (`SessionMachine::commit_open` builds a fresh + `ActiveSession` with `next_wire_seq: 0`) and advances by exactly 1 only via + `commit_probe_sent`, after a confirmed successful socket send. TX + timestamping is enabled after that same successful Open and before any probe + is sent, so the normal zero points line up. - **One socket, one session, no reopen.** `SessionMachine`'s state machine only reaches `Open` from `Connected`, and `commit_open`/`commit_local_close` never route back to `Connected`. A `Client`/`AsyncClient` cannot reopen a diff --git a/crates/irtt-client/src/async_client.rs b/crates/irtt-client/src/async_client.rs index 2c2bfa6..c531c47 100644 --- a/crates/irtt-client/src/async_client.rs +++ b/crates/irtt-client/src/async_client.rs @@ -608,8 +608,14 @@ impl AsyncClient { // No sent_at measurement, no machine commit, no schedule // commit: the transaction is fully discarded and retried // with a fresh send_anchor on the next loop iteration. - Err(error) if error.kind() == io::ErrorKind::WouldBlock => continue, - Err(error) => return Poll::Ready(Err(ClientError::Socket(error))), + Err(error) if error.kind() == io::ErrorKind::WouldBlock => { + self.machine.invalidate_kernel_tx_correlation(); + continue; + } + Err(error) => { + self.machine.invalidate_kernel_tx_correlation(); + return Poll::Ready(Err(ClientError::Socket(error))); + } }; #[cfg(not(test))] diff --git a/crates/irtt-client/src/client.rs b/crates/irtt-client/src/client.rs index 3ada7f5..2d18a5d 100644 --- a/crates/irtt-client/src/client.rs +++ b/crates/irtt-client/src/client.rs @@ -163,12 +163,13 @@ impl Client { /// /// On success, returns the negotiated open outcome and transitions the /// client into either an open probe session or completed no-test state. - /// Open attempts use [`ClientConfig::open_timeouts`]. Malformed, unrelated, - /// or unauthenticated datagrams are ignored until the current attempt's - /// absolute deadline, so one attempt may consume several datagrams without + /// Open attempts use [`ClientConfig::open_timeouts`]. Malformed or + /// unrelated datagrams are ignored until the current attempt's absolute + /// deadline, so one attempt may consume several datagrams without /// retransmitting. Silence or ignored traffic eventually produces - /// [`ClientError::OpenTimeout`], while authenticated incompatibility remains - /// terminal. + /// [`ClientError::OpenTimeout`]. A structurally recognizable malformed or + /// incompatible Open reply from the connected peer is terminal; when HMAC + /// is configured, that recognition additionally requires authentication. /// /// When a trusted reply allocates a token but later negotiation or socket /// preparation fails, the client sends a best-effort cleanup close and @@ -523,11 +524,18 @@ impl Client { .unwrap_or_else(Instant::now); #[cfg(test)] if fail_send { + runtime.invalidate_kernel_tx_correlation(); return Err(ClientError::Socket(io::Error::other( "injected probe send failure", ))); } - let bytes = socket.send(&prepared.bytes)?; + let bytes = match socket.send(&prepared.bytes) { + Ok(bytes) => bytes, + Err(error) => { + runtime.invalidate_kernel_tx_correlation(); + return Err(ClientError::Socket(error)); + } + }; #[cfg(not(test))] let send_finished_at = Instant::now(); #[cfg(test)] diff --git a/crates/irtt-client/src/probe.rs b/crates/irtt-client/src/probe.rs index 6a5d58c..4d035cf 100644 --- a/crates/irtt-client/src/probe.rs +++ b/crates/irtt-client/src/probe.rs @@ -39,7 +39,8 @@ pub(crate) struct PendingProbe { /// eligible only for upstream one-way delay after local plausibility /// validation against `tx_not_before_wall` (see `compute_one_way`'s /// `preferred_send_wall`). `sent_at` remains the userspace fallback for - /// upstream one-way delay when this is absent or implausible. + /// upstream one-way delay when this is absent, implausible, or kernel-ID + /// correlation was invalidated by a failed probe submission. pub kernel_tx_timestamp: Option, } diff --git a/crates/irtt-client/src/receive/linux.rs b/crates/irtt-client/src/receive/linux.rs index cdefb6b..53c9a53 100644 --- a/crates/irtt-client/src/receive/linux.rs +++ b/crates/irtt-client/src/receive/linux.rs @@ -12,14 +12,17 @@ //! After a successful Open, the client adapter best-effort upgrades the //! socket from [`RX_TIMESTAMPING_FLAGS`] to [`error_queue::TX_TIMESTAMPING_FLAGS`] //! via [`try_enable_tx_timestamping`]. When that succeeds, the automatic -//! `SOF_TIMESTAMPING_OPT_ID` counter the kernel assigns to each successfully -//! submitted datagram is used directly as the probe's wire sequence number -//! (see the client crate's `AGENTS.md` for the correlation invariant this -//! relies on). [`drain_tx_timestamps`] performs a small bounded, nonblocking -//! read of `MSG_ERRQUEUE` so the adapter can opportunistically collect those -//! timestamps without ever waiting for one. The observed kernel TX wall time -//! is retained as metadata only; it is not consumed by any measurement in -//! this change. +//! `SOF_TIMESTAMPING_OPT_ID` counter normally tracks the probe wire sequence +//! after successful sends, so it is used as a best-effort correlation ID. +//! A kernel ID can theoretically be consumed by a send that later fails, +//! desynchronizing that correlation. [`drain_tx_timestamps`] discards +//! unmatched or implausible records; the userspace `sent_at` timestamp remains +//! the fallback. After a probe send failure, correlation is disabled for that +//! session so later IDs cannot be misattributed. It performs a small bounded, +//! nonblocking read of +//! `MSG_ERRQUEUE` so the adapter can opportunistically collect timestamps +//! without ever waiting for one. The observed kernel TX wall time is retained +//! as metadata only; it is not consumed by any measurement in this change. use std::{ io::{self, IoSliceMut}, diff --git a/crates/irtt-client/src/receive/linux/error_queue.rs b/crates/irtt-client/src/receive/linux/error_queue.rs index c224919..8d16e65 100644 --- a/crates/irtt-client/src/receive/linux/error_queue.rs +++ b/crates/irtt-client/src/receive/linux/error_queue.rs @@ -624,22 +624,14 @@ mod tests { )); } - // NOTE: a live test proving that a failed nonblocking `sendmsg` does not - // consume an automatic OPT_ID is not included here. Forcing a reliable, - // non-flaky `WouldBlock` on a connected loopback UDP socket (e.g. via - // socket-buffer saturation) has no deterministic OS-level mechanism - // available in this environment, and the repository's testing policy - // explicitly forbids faking coverage with sleep/saturation-based - // flakiness. That safety is instead established from Linux kernel - // source: `_sock_tx_timestamp` (include/net/sock.h) and - // `__ip_append_data`/`__ip6_append_data` (net/ipv4/ip_output.c, - // net/ipv6/ip6_output.c) increment `sk->sk_tskey` speculatively before - // a datagram is fully built and explicitly `atomic_dec` it back out on - // every error path (tracked via a local `hold_tskey` flag), so a failed - // send can never leave a gap in — or otherwise advance — the ID space - // the kernel hands out to successfully submitted datagrams. See the - // client crate's `AGENTS.md` for the full citation. The client-side - // half of this invariant (a `WouldBlock` `try_send` never advances the - // local wire-sequence counter used as the correlation ID) is covered by - // a deterministic test in the adapter module that injects `WouldBlock`. + // NOTE: There is no live test for whether a failed nonblocking `sendmsg` + // consumes an automatic OPT_ID. Forcing a reliable, non-flaky `WouldBlock` + // on a connected loopback UDP socket (for example through socket-buffer + // saturation) has no deterministic OS-level mechanism in this environment, + // and the repository's testing policy forbids sleep/saturation-based + // flakiness. The correlation is intentionally best effort: a kernel ID can + // theoretically be consumed by a send that later fails. The deterministic + // adapter test covers only the local half: an injected `WouldBlock` does + // not advance the local wire-sequence counter. Any later unmatched kernel + // timestamp is discarded and the userspace `sent_at` remains the fallback. } diff --git a/crates/irtt-client/src/session/machine.rs b/crates/irtt-client/src/session/machine.rs index e80fc09..4398d9d 100644 --- a/crates/irtt-client/src/session/machine.rs +++ b/crates/irtt-client/src/session/machine.rs @@ -60,6 +60,7 @@ struct ActiveSession { pending: PendingMap, timed_out: TimedOutMap, completed: CompletedSet, + kernel_tx_correlation_valid: bool, } #[derive(Debug)] @@ -423,15 +424,27 @@ impl SessionMachine { } } + /// Disable Linux kernel TX timestamp correlation after a probe send fails. + /// + /// A failed submission can consume a kernel `SOF_TIMESTAMPING_OPT_ID` + /// without advancing the wire sequence. Existing associated timestamps + /// remain valid observations; later error-queue timestamps cannot be + /// associated safely and are ignored for the rest of this session. + pub(crate) fn invalidate_kernel_tx_correlation(&mut self) { + if let MachineState::Open(session) = &mut self.state { + session.kernel_tx_correlation_valid = false; + } + } + /// Record an observed Linux kernel TX timestamp for `wire_seq`, the /// automatic `SOF_TIMESTAMPING_OPT_ID` the kernel assigned when the /// datagram was submitted. /// - /// `wire_seq` doubles as that correlation ID: both counters start at - /// zero for a session's first timestamped send and advance by exactly - /// one only on a confirmed successful submission, so they stay - /// identical for the life of one open session (see the client crate's - /// `AGENTS.md` for the full invariant). + /// `wire_seq` normally matches that correlation ID after successful + /// sends. This is best effort rather than an invariant: a kernel ID can + /// theoretically be consumed by a send that later fails, leaving the + /// counters desynchronized. An unmatched or implausible ID is discarded; + /// the probe's userspace `sent_at` remains the fallback. /// /// Updates a still-pending or already-timed-out probe in place. Never /// resurrects a completed, evicted, or unknown probe, and never touches @@ -443,6 +456,9 @@ impl SessionMachine { let MachineState::Open(session) = &mut self.state else { return; }; + if !session.kernel_tx_correlation_valid { + return; + } if let Some(probe) = session.pending.get_mut(wire_seq) { probe.kernel_tx_timestamp.get_or_insert(timestamp); return; @@ -636,6 +652,7 @@ impl SessionMachine { pending: PendingMap::new(self.config.max_pending_probes), timed_out: TimedOutMap::new(self.config.max_pending_probes), completed: CompletedSet::new(self.config.max_pending_probes), + kernel_tx_correlation_valid: true, })); let event = ClientEvent::SessionStarted { @@ -1231,6 +1248,7 @@ mod tests { pending: PendingMap::new(max_pending_probes), timed_out: TimedOutMap::new(max_pending_probes), completed: CompletedSet::new(max_pending_probes), + kernel_tx_correlation_valid: true, })); machine } @@ -2873,6 +2891,25 @@ mod tests { ); } + #[test] + fn invalidated_kernel_tx_correlation_uses_userspace_send_time() { + let mono = Instant::now(); + let mut machine = upstream_probe(send_wall_ms(1_000), mono); + + // A failed send can consume a kernel ID without advancing wire_seq. + // A later timestamp whose ID happens to match a pending wire sequence + // must not be associated after that gap. + machine.invalidate_kernel_tx_correlation(); + machine.record_kernel_tx_timestamp(0, send_wall_ms(1_005)); + + let events = upstream_reply(&mut machine, 1_020, send_wall_ms(1_040), mono); + + assert_eq!( + reply_one_way(&events).unwrap().client_to_server, + Some(SignedDuration::from_nanos(20_000_000)) + ); + } + #[test] fn upstream_one_way_delay_falls_back_without_kernel_tx() { let mono = Instant::now(); @@ -3195,6 +3232,7 @@ mod tests { pending: PendingMap::new(MAX_PENDING_PROBES), timed_out: TimedOutMap::new(MAX_PENDING_PROBES), completed: CompletedSet::new(MAX_PENDING_PROBES), + kernel_tx_correlation_valid: true, })); (machine, params) } diff --git a/crates/irtt-server/AGENTS.md b/crates/irtt-server/AGENTS.md index 576be03..d3804b3 100644 --- a/crates/irtt-server/AGENTS.md +++ b/crates/irtt-server/AGENTS.md @@ -500,13 +500,14 @@ session policy lives. unmarked reply into a *dropped* one, because an unappliable marking stops the send. Verify a platform before extending that list. - A genuinely dual-stack `[::]` listener still emits **unmarked** IPv4-mapped - replies, and this is known. It is not fixed by choosing the option per reply, - for the macOS reason above, and it predates wildcard source selection. Leave it - until it is worth its own slice, and do not "fix" it by weakening the rule that - an unappliable marking stops the send. The crate forbids unsafe code; - there is no raw `setsockopt` and no `sendmsg`/`cmsg` machinery. Keep the - target lists identical to the pinned `socket2` version's own gates — check - its source rather than copying another crate's older matrix. + replies on Linux. When reply DSCP/traffic-class marking for IPv4 peers + matters, configure the normal same-port `[::]:PORT` plus `0.0.0.0:PORT` + listener pair instead; `ServerSet` makes the IPv6 half IPv6-only for that + pair. Do not force `IPV6_V6ONLY` on every lone IPv6 listener, and do not + weaken the rule that an unappliable marking stops the send. The crate forbids + unsafe code; there is no raw `setsockopt` and no `sendmsg`/`cmsg` machinery. + Keep the target lists identical to the pinned `socket2` version's own gates + — check its source rather than copying another crate's older matrix. ## Rate, lifetime and server-initiated close diff --git a/crates/irtt-server/src/set.rs b/crates/irtt-server/src/set.rs index 9e7566b..b3efc68 100644 --- a/crates/irtt-server/src/set.rs +++ b/crates/irtt-server/src/set.rs @@ -91,6 +91,14 @@ impl ServerSet { /// An IPv4-mapped address such as `[::ffff:0.0.0.0]` is an IPv4 listener /// here, not an IPv6 one, matching how the runtime already reads it. /// + /// A lone explicit dual-stack `[::]:PORT` listener is not the recommended + /// configuration when reply DSCP/traffic-class marking for IPv4 peers + /// matters: Linux serves those peers as IPv4-mapped and cannot retain the + /// negotiated marking on their replies. Request the normal same-port + /// `[::]:PORT` and `0.0.0.0:PORT` pair instead. This method makes the IPv6 + /// half IPv6-only for that pair only; a lone IPv6 listener retains its + /// normal socket behavior. + /// /// # Errors /// /// Returns [`ServerSetError::NoListeners`] if `addrs` is empty — a set diff --git a/docs/irtt-server.md b/docs/irtt-server.md index 92958e5..f69be58 100644 --- a/docs/irtt-server.md +++ b/docs/irtt-server.md @@ -63,6 +63,15 @@ different ports. With no `--bind` at all, see [Description](#description) above for the default wildcard pair and the address-family fallback it allows. +When reply DSCP/traffic-class marking for IPv4 peers matters, do not configure +one explicit dual-stack `[::]:PORT` listener by itself. On Linux, IPv4 peers +served through that socket are IPv4-mapped and their replies cannot retain the +negotiated marking. Configure the ordinary paired `[::]:PORT` and +`0.0.0.0:PORT` listeners instead; the IPv6 listener is made IPv6-only for that +same-port pair, so each family uses the correct socket option. This is a +configuration workaround, not a reason to force `IPV6_V6ONLY` for every lone +IPv6 listener. + ### `--hmac KEY` HMAC key, taken as the UTF-8 bytes of this argument. With a key configured,