Subject: [quicwg/base-drafts] Processing buffered packets can cause hugely inflated RTT measurements (#3980)

From: ianswett <[email-masked]>
Date: Mon, 03 Aug 2020 07:50:46 -0700

I'm splitting this from #3821 because the set of changes are disjoint from the current PR for that issue and I think having both issues in one is making it harder to follow. (The rest of this text is an edit of my last post on that issue).

The issue is that ACKs for Handshake and 1-RTT packets could be delayed because the peer doesn't have the keys(either because it takes time to derive them, or it's missing crypto data to derive the next key).

The first RTT sample in Handshake and 1-RTT packet spaces could have substantial delay in excess of the 'no ack delay' in Handshake or max_ack_delay for 1-RTT, which inflates SRTT and RTTVar.

If there's no loss, this fixes itself without harm, which is nice. And it's not as bad for ApplicationData, since that will have a non-0 max_ack_delay, which may be enough to absorb the endpoint delay generating keys or doing cert verification, though it won't typically be enough to adjust for a packet being lost and the data being PTO'd and then that making keys available.

In #3921, Jana's suggested changing ack_delay to including the time packets were buffered as well as and ignoring max_ack_delay for the first sample in a PN space.  I think this is a sensible mitigation, though it doesn't guarantee the issue will not occur.

If no Initial ACKs have been received and the ACK with the large ack_delay is the first ACK of the connection, then you still can't use ack_delay.

The one change we've found mitigates this issue entirely is coalescing packets from the next PN space(Handshake or 1RTT) and then processing the full coalesced packet before processing all buffered packets and sending an ACK. That ensures there's one new packet being acknowledged and you get a non-delayed RTT sample. It doesn't seem onerous to add a "SHOULD process all packets in the current datagram and all buffered packets before sending an ACK to avoid generating multiple inflated RTT samples" to me?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980

---

From: Martin Thomson <[email-masked]>
Date: Mon, 03 Aug 2020 18:41:41 -0700

Repeating this here, so that the PR comments don't get lost:

This is about a sender having to buffer packets that might contain ACK frames.  Usually, this is because the sender doesn't have the keys necessary, so they hold the packets until they do.

When processing the ACK frame, some time might have passed.  So we now have 5 times in play:  when the largest acknowledged packet was a) sent, and b) received; and when the packet containing the ACK was c) sent, d) buffered, and e) processed.  The critical difference here is that we now recognize the intentional delays at the sender in addition to intentional delays at the receiver.

The receiver calculates and transmit `c - b` and sends that in ACK Delay.  We have a commitment that `c - b <= max_ack_delay`.  That doesn't need to change.

The spec currently assumes that the sender calculates RTT as `e - a - (c - b)` (or `e - a - min(c - b, max_ack_delay)` to include the cap on receiver delay).  That is, the time between when the ACK is processed and when the largest acknowledged was sent, less the ACK Delay.  But that's wrong.  It's `d - a - (c - b)` that we care about.

So all we need to say is that if the sender buffers packets for processing, then it needs to calculate RTT based on the time that the packet entered the buffer.  That is, we capture any intentional delays added *by the sender*.

Then we don't have to worry about the max_ack_delay cap, because receiver behaviour always follows the contract.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-668328330

---

From: Martin Thomson <[email-masked]>
Date: Mon, 03 Aug 2020 21:50:39 -0700

Sorry, I should correct that last bit:  We only have to worry about the max_ack_delay cap when the packet that is being acknowledged might have been buffered.  Thus, it makes sense to remove the cap for packets that might have been buffered (i.e., those that are sent prior to handshake confirmation).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-668377696

---

From: ianswett <[email-masked]>
Date: Tue, 04 Aug 2020 08:25:22 -0700

Also from the PR.  I had never considered the case when the sender has buffered a packet that contains an ACK frame until you pointed it out.  The PR was intended to deal with the case when a peer receives packets it can't immediately decrypt and acknowledge.

But I added a sentence about subtracting the amount of time buffered when processing a buffered packet containing an ACK.  It can only happen for a server that hasn't reached handshake confirmed, so it's fairly rare.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-668662719

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 04 Aug 2020 13:45:19 -0700

It seems to me that this issue is an alternative resolution to one specific part of what is being proposed as the resolution to #3821. Specifically, quoting from https://github.com/quicwg/base-drafts/issues/3821#issuecomment-665393532, the approach proposed here is an alternative to the line being emphasized below.

> S1. To solve the first problem, we already have a condition in the recovery draft to _not_ arm the PTO on
1-RTT data until the handshake is complete (Section 6.2.1). This is not quite adequate, since it still allows
a client to arm a PTO timer on 1-RTT data on sending a CFIN and 1-RTT data, before ensuring that the server
has received the CFIN. The proposed solution here is to change this condition from "handshake complete" to
"handshake confirmed". Meaning that an endpoint only arms a PTO timer on 0-RTT or 1-RTT data when it knows
that the peer has the keys necessary to read and write in 1-RTT, and can therefore respond with an ACK.
> 
> We think this holds up, but we'd like to get this under everyone's eyes, to ensure that the discussion isn't
missing subtle deadlock issues.
> 
> S2. To solve the second problem, the proposal is to use ack_delay as reported by the endpoint that buffers
the data appropriately. ack_delay is the amount of the time the information (regardless of that being the
packet itself or the acknowledgement) being buffered intentionally. For this, we need the following:
> 
> - endpoints MUST report an ack_delay that is the sum of the controlled delay (i.e. the duration of the data
and the ack being buffered)
> 
> - __endpoints ignore max_ack_delay for acknowledgements of packets that were sent prior to handshake
confirmation. Those are the ones that might have been buffered at the peer until keys were ready.__
> 
> In addition, we explicitly recommend endpoints that asynchronously process handshake messages to buffer
1-RTT packets, instead of dropping them, as dropping packets has significant impact on congestion control.
Note that this matches what those implementations already do.

The proposed resolution in #3821 recommends the sender to retain one bit of constant for each packet it sends, and use it to see if ack_delay should be capped.

The resolution proposed here is to retain a mutable bit for each packet number space, and flip that bit when the seder receives the first ack. Until that bit is flipped, the sender does not cap ack_delay. As correctly pointed out in the [original comment](https://github.com/quicwg/base-drafts/issues/3980#issue-672131960), this approach relies on the receiver processing all the buffered packets at once then sending an ACK.

I think that both approaches work, and do not have a strong preference among the two.

I'd also note both approaches would work correctly once we adopt the basis of the latter approach; i.e. recommend the receiver to process all buffered packets before sending an ACK (the basis of the latter resolution.

Therefore, it seems to me that the only question is if we want to have that recommendation. If the answer is yes, then both approaches would work, and the choice between the approaches becomes an editorial preference (i.e. write down either or both).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-668816044

---

From: ianswett <[email-masked]>
Date: Tue, 04 Aug 2020 14:16:28 -0700

I actually updated the PR to align more closely with the original proposal after @mt pointed out that they weren't identical if all buffered packets weren't processed at once(which is something I'd implicitly assumed).  I could go either way on that, but I do think it's easier to only special case the first ACK for a PN space rather than store a bit per packet.  It also mitigates the issue for those that don't follow this new MAY.

However, one difference from the above is this doesn't rely on handshake confirmation, since this is just as much a problem for Handshake packets as it is 1-RTT packets, so I think we need to use a more general statement about the sender not knowing if the receiver has the keys to process the packets.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-668829194

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 04 Aug 2020 15:08:08 -0700

@ianswett (swapping paragraphs being quoted)
> However, one difference from the above is this doesn't rely on handshake confirmation, since this is just as much a problem for Handshake packets as it is 1-RTT packets, so I think we need to use a…

I am happy to see the issue addressed for all epochs if that's the preference of others.

> It also mitigates the issue for those that don't follow this new MAY.

FWIW I tend to think that we should use SHOULD, especially if we are going to also cover the delay of Handshake ACKs.

Max-ack-delay of Handshake packets are assumed to be zero. Therefore, we would see the problem on any environment where the time required for processing a TLS handshake message is relatively greater than the path delay. Including datacenter networks where RTT is small, or slow IoT devices.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-668848535

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Aug 2020 15:05:33 -0700

Let's mark this as design and run the process.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3980#issuecomment-672305115