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?
2 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.