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

Wrongly hinting of issues not existing for packet number size increase #605

Closed
gloinul opened this issue Jun 7, 2017 · 11 comments
Closed
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@gloinul
Copy link
Contributor

gloinul commented Jun 7, 2017

draft-ietf-quic-transport-03, section 5.8:

The sender MUST use a packet number size able to represent more than
twice as large a range than the difference between the largest
acknowledged packet and packet number being sent. A peer receiving
the packet will then correctly decode the packet number, unless the
packet is delayed in transit such that it arrives after many higher-
numbered packets have been received. An endpoint MAY use a larger
packet number size to safeguard against such reordering.

I don't quite understand the second sentence here. If I understand the first sentence with the MUST, that forces the sender to increase the packet number least significant bit with additional byte(s) to fit the outstanding part of the sequence number space so that it doesn't wrap when having outstanding packets from the senders perspective. Thus, even the very old packet case should only force the sender to increase the size of the field while it is outstanding. There shouldn't be a uncertainty for the receiver. This as the higher bits added will match the receivers representation for the highest acked packet number without any outstanding packet prior to it.

@mikkelfj
Copy link
Contributor

mikkelfj commented Jun 7, 2017

The sender cannot control network properties. There might be some old already retransmitted packets suddenly resurfacing, albeit unlikely.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Jun 7, 2017
@martinthomson martinthomson changed the title Transport: Wrongly hinting of issues not existing for packet number size increase Wrongly hinting of issues not existing for packet number size increase Jun 20, 2017
@martinthomson
Copy link
Member

There is a real issue here. Say that a sender sends 100 packets with 8-bit packet numbers. That's perfectly OK according to the rules in the draft.

If the sender then immediately send another 1000 packets while those 100 are outstanding, the new packets will have 16-bit packet numbers. But the sender can't retroactively increase the size of the packet numbers of packets already sent. And those packets might be reordered with respect to these newer packets.

If a packet from the set with 8-bit packet numbers is delayed and arrives after receiving a higher numbered packet from the 1000 packets, then the shorter packet number could be decoded incorrectly. For example, assuming the recommended decoding algorithm, packet 16 encoded on 8 bits will be incorrectly decoded as packet 272 if packet 144 or higher has been received.

@mcmanus
Copy link
Contributor

mcmanus commented Jun 20, 2017

yes; you can be semi-blocked from sending by the range of the lowest unacked packet you have outstanding.

I say semi-blocked (as opposed to say, deadlocked) because you can just move to the bigger packet number space and essentially declare the older uacked ones presumed-lost. They'll fail decryption at the receiver when given the 'wrong' packet number.

8 bit numbers aren't especially useful.. perhaps banning them as footguns would be a practical step to help quality of implementations.

@mikkelfj
Copy link
Contributor

I say semi-blocked (as opposed to say, deadlocked) because you can just move to the bigger packet number space and essentially declare the older uacked ones presumed-lost.

This will not work if it is decided to require gapless packet numbers except on path migration - something I'd argue for.

#595 suggests variable length integers which I think is a good idea.

@mcmanus
Copy link
Contributor

mcmanus commented Jun 20, 2017

I don't think gapless creates a problem with the existing spec (though I also support #595). Maybe I'm misunderstanding you.

Gap is a property of the sender, and there would be no gap just by virtue of moving to a bigger encoding - that's obvious to the receiver if the packets are delivered in order but its true whether the receiver realizes it or not. So out of order delivery would basically just be a form of loss/corruption (the misinterpreted number would cause a decrypt fail) fixed in the usual way (retransmit of data).

To me, the most important part of this issue is that the current design is prone to this self-induced loss and while not a critical fail, it should be improved.

@mikkelfj
Copy link
Contributor

mikkelfj commented Jun 20, 2017

Maybe I'm misunderstanding you.

Or may be the opposite - I thought you would introduce a gap in the packet number space while advancing the larger range, but that no longer makes sense to me.

Either way, I'm not sure there is a problem:

If you send 100 packets in 8-bit encoding, then decide to send 100000 packets more, you encode 8 bit until packet 127, use 16 bit until packet 32767, and 32 bit after that.

Regardless of delivery order, there shouldn't be any ambiguity about the packer number.

But this isn't exactly how things are specified currently - it uses a diff against largest received or something like that.

If the spec would state that only half the representable range may be used for any given encoding, except for 64 bit, then I think it should be working.

Simpler yet, use varlen.

EDIT: actually, you can use the full range of encoding without any issues.

@gloinul
Copy link
Contributor Author

gloinul commented Jun 20, 2017

@mikkelfj I think you are correct that this can be resolved by changing the rules for how one handle or recover the full packet number. I think the current text actually requires one to not send any more packets when one reaching having half the current smallest packet field space as diff between next packet and oldest outstanding. Thus, one could actually deadlock, which is bad.

If one statys with the current style of the packet numbers, I would agree with @mcmanus that the 8-bit filed is likely unnecessary.

@gloinul gloinul closed this as completed Jun 20, 2017
@gloinul gloinul reopened this Jun 20, 2017
@ianswett
Copy link
Contributor

ianswett commented Jun 20, 2017

@martinthomson The concern you point out is real, though as @mcmanus points out, it just causes a few packets to be dropped when they can't be decrypted in the worst case.

Reordering is somewhat common, but reordering of >100 packets is pretty rare, so even 1 byte works very well in the real world.

<Old comment edited, as I found the section on packet numbers in the current draft>

@ianswett
Copy link
Contributor

After re-reading the existing text, I think we need to add some text about the largest packet number likely to be sent before a larger packet is acknowledged.

Specifically, I'd change "An endpoint MAY use a larger packet number size to safeguard against such reordering." to "An endpoint SHOULD use a large enough packet number size to allow any packets sent in the near future be unambiguously decoded."

Also @gloinul , I can't find any text that would create a deadlock. The only MUST is: "The sender MUST use a packet number size able to represent more than twice as large a range than the difference between the largest acknowledged packet and packet number being sent."

@gloinul
Copy link
Contributor Author

gloinul commented Jun 21, 2017

@ianswett sorry for being unclear. I don't think the limitation do exist in the text to cause an actual deadlock. I meant if one would enforce to changes of packet number to happen so that no receiver confusion would be possible then a deadlock could occur.

What I think needs to happen is that one clarify a couple of things:

  1. When one increase the packet number field size, that can happen at any time because the number of outstanding packets becomes to large.
  2. When one increase, then I think the receiver processing should be clarified, so that it would be clear how the shorter packet numbers relates to the larger ones to avoid the issue of wrongly reconstructing the full packet number.
  3. The conditions in which one can shrink the packet number field needs to be clear so that one minimizes the risk that packets gets wrongly reconstructed packet numbers. These recommendations will of course need to take 2. into account.

@ianswett
Copy link
Contributor

The text for 1 and 3 should be the same, since it's just a matter of determining what the right length is at any given moment. I don't think anything special needs to be noted about increases or decreases, but Martin's PR should make it clearer how to pick a length.

Re 2, I think the receiver processing is quite simple and works correctly without any special language about shorter vs longer numbers, so I'd prefer to avoid it. Is there a specific case you think is confusing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

No branches or pull requests

5 participants