Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More ECN verification failure modes #2198

Closed
martinthomson opened this issue Dec 17, 2018 · 4 comments
Closed

More ECN verification failure modes #2198

martinthomson opened this issue Dec 17, 2018 · 4 comments
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@martinthomson
Copy link
Member

(From #2189)

There's a perverse situation in which ECN verification results in another problem.

That situation is where the largest acknowledged is lower on packet that is sent later. AND that packet acknowledges more packets so that the counts are higher. AND that packet is reordered ahead of packets with higher largest acknowledged. This results in the verification failing on the second packet to arrive (counts are lower because it's an older ACK, but the largest acknowledged is higher).

A fix is to never decrease the largest acknowledged when sending an ACK frame. Another is to disable validation if the packet containing the ACK is not higher-numbered than the last packet containing an ACK. The question is whether we care enough to recommend either fix...

@gloinul, WDYT?

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Dec 17, 2018
@gloinul
Copy link
Contributor

gloinul commented Dec 17, 2018

Maybe we need to bring back my proposed text to actually look that the PN of the packet containing the ACK frame is the highest received so far, else do not perform ECN verification. It requires a single state variable, and likely one per packet status boolean (wasReordered) to use during processing.
This I think got removed for more shorter and "elegant" text. That way all these reording things goes away. Without the reordering the ECN counters will never go backwards, even if an ACK, only ACKs what was an gap in what been acked so far.

@gloinul
Copy link
Contributor

gloinul commented Dec 19, 2018

#2205 if there is a requirement for the ACK frame to always include a largest acknoweldge equal to largest received then the above can't happen. The the later reordered packet will have the same largest acknowledged as the later arriving, but sent later. Thus, the ECN verification check would be that Largest Acknowledged MUST increase for verification to happen. Or one actually looks at the PN of the packet of the ACK frame to ensure that they arrive in order.

@gloinul
Copy link
Contributor

gloinul commented Dec 20, 2018

I think the best in this case is to not preform vierfication for out of order ACKs.

The current sentence in Section 13.3.2 (-17):
"Counts MUST NOT be verified if the ACK frame does not increase the largest received packet number at the endpoint."

Which I interpret to mean that what PN the ACK reference (sender-receiver direction), rather than the PN of the receiver to sender direction. Can you @martinthomson fix that? That way #2205 can focus on the other aspect and ignore ECN as it would not care.

@ianswett
Copy link
Contributor

@gloinul That would be my preference as well.

ianswett added a commit that referenced this issue Dec 20, 2018
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

4 participants