Don't gate PTO packets on address validation - #49
Conversation
At the moment the code will not send an ack-eliciting ping once the peer's address is validated. This appears to be linked to RFC 9000 Section 8.1, which states > To prevent this deadlock, clients MUST send a packet on a Probe > Timeout (PTO) [...] the client MUST send an Initial packet in a UDP > datagram that contains at least 1200 bytes if it does not have > Handshake keys, and otherwise send a Handshake packet. However I think this is a slight mis-read, and that RFC 9002 Section 6.2.4 > When a PTO timer expires, a sender MUST send at least one ack-eliciting > packet [...] When there is no data to send, the sender SHOULD send a > PING or other ack-eliciting frame in a single packet, rearming the PTO > timer. shows that whilst validation is an important inflection point for the two behaviors, PTO pings should still be sent after it. The pseudocode in A9 I think also indicates this. A new test with the current code results in outstanding frames which will never have a PTO fire and the current code will end up in a tight loop. This change removes the validation distinction in `sendPTO`, along with the now-unused validation local and the timer-clear branch it fed; the difference in behavior required by the RFC will be enacted by the timer.
PTO can generate PINGs still if the ACK blocks get large enough to piggyback a PING frame to make them ACK-eliciting, see the code here: https://github.com/apple/swift-network-evolution/blob/main/Sources/SwiftNetwork/QUIC/Ack.swift#L304C13-L304C24 |
| if ackElicitingPacketsInFlight == 0 { | ||
| guard ackElicitingPacketsInFlight > 0 else { | ||
| if _slowPath(ackElicitingPacketsInFlight < 0) { | ||
| connection.log.fault("ackElicitingPacketsInFlight negative: \(ackElicitingPacketsInFlight)") |
There was a problem hiding this comment.
We should probably make sure we are not underflowing ackElicitingPacketsInFlight, possibly adding an underflow check to -= 1 since this is an integer. That way we do not have to log out these negative values.
| if !sentPTO { | ||
| connection.log.datapath("Sending a PING as PTO") | ||
| if peerCompletedValidation { | ||
| connection.log.fault("PTO fired after validation") |
There was a problem hiding this comment.
Historically this log has been a warning that something in Recovery may not be working as expected, or you may have run into a race condition in the logic. For example, I am looking at a case now where ackElicitingPacketsInFlight is 1, but then the handshake completes, clears the ack-eliciting packets, resetPTOCount set the count back 0, and then sendPTO falls into this path as a matter of timing. When this happens sendPTO tries to reset the timer and realizes that there are now no ackElicitingPacketsInFlight and then cancels the timer and continues on as normal.
So in this path we have now removed the resetTimer functionality, what is the rationale there?
There was a problem hiding this comment.
I don't think we have lost the timer. By my read sendPTO's only caller is timerFired, which calls resetTimer right after it returns, and that cancels when nothing is in flight and the peer has validated us. So I think the existing shouldClearTimer cancel was redundant.
I think your callout about the potential race is right and I regressed that here. With nothing in flight the loop does nothing, so the unconditional fallback would have sent a spurious PING. It's now gated on whether anything is in flight rather than on validation. That case keeps the fault and sends nothing, and the probe is only unconditional when there's something to probe for.
|
As discussed with @agnosticdev, the code was assuming that if we hadn't already finished peer validation (i.e., we were in the middle of the handshake), we should have had ack eliciting packets to send, but it seems like that may not be true when the timer is scheduled in conjunction with the receipt of an ACK. |
`sendPTO` chose how to probe using state that did not match what it was probing. `hasAckElicitingPendingItems` was read once for the space returned by `getEarliestTime` and then applied to *every space*. I don't think this should be the case because pending items are held per space. An error in the previous commit meant that the fallback probed unconditionally, so a PTO that fired with nothing in flight still sent a PING. This happens when the handshake completes and clears the in-flight packets after the timer was armed. `ackElicitingPacketsInFlight` no longer decrements below zero, so the read side does not need to detect negative values. The all-spaces sum that `sendPTO` and `resetTimer` both consult is now a shared property rather than duplicated. The two PTO tests now assert that a probe was recorded rather than that the PTO was counted, since the count advances on the attempt. They cover the two branches that can produce no packet: a tail retransmit that cannot be rebuilt, and new data that writes no payload.
Resolved a conflict in `sendPTO` with 410ec51 ("Stop logging a fault for valid PTO cases"), which split the `PTO fired after validation` log into error and fault while keeping the validation gate. Kept this branch's behaviour, which probes when packets are in flight. The case that commit faulted for, nothing in flight after validation, still faults and sends nothing. Took main's `inout` `recordSentPackets` from 32fe031 in place of this branch's `removeFirst()` change.
Yep, my initial wording was too strong here. They can be sent in that case, I meant this last-resort PING. That highlighted case only fires a PING for specific traffic patterns - it needs inbound traffic gappy enough to produce more than 5 ACK blocks, and the flag is consumed on write. With no inbound traffic this reproduces deterministically. |
|
I've pushed a follow-up commit for the review comments, plus two issues they turned up, and merged in main. Both The PTO fired after validation path is no longer unconditional. It's gated on whether anything is The other issues I noticed were in the same function.
I checked each test fails only for its own defect, by reverting each fix separately. The merge conflicted with #62 ("Stop logging a fault for valid PTO cases") and I resolved in favour of |
| if !sentPTO { | ||
| if totalAckElicitingPacketsInFlight == 0, peerCompletedValidation(connection: connection) { | ||
| // Nothing is in flight to probe for, so the state must have changed between arming the | ||
| // timer and it firing (e.g. the handshake completed and cleared the in-flight packets). |
There was a problem hiding this comment.
Yep, thank you for calling this condition out.
3 participants
At the moment the code will not send an ack-eliciting ping once the peer's address is validated. This appears to be linked to RFC 9000 Section 8.1, which states
However I think this is a slight mis-read, and that RFC 9002 Section 6.2.4
shows that whilst validation is an important inflection point for the two behaviors, PTO pings should still be sent after it. The pseudocode in A9 I think also indicates this.
A new test with the current code results in outstanding frames which will never have a PTO fire and the current code will end up in a tight loop.
This change removes the validation distinction in
sendPTO, along with the now-unused validation local and the timer-clear branch it fed; the difference in behavior required by the RFC will be enacted by the timer.